Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split up babel-core's File class and add Flowtype annotations #6359

Merged
merged 3 commits into from
Oct 2, 2017

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues
Patch: Bug Fix? N
Major: Breaking Change? N, unless you're using undocumented methods/props on File.
Minor: New Feature?
Tests Added/Pass?
Spec Compliancy?
License MIT
Doc PR
Any Dependency Changes?

FYI, may be easiest to review by commit. Mostly refactoring. I've removed the parts of the File class that weren't part of the user-facing API and moved then to be their own files.

Actual features in here:

  • Added browser files for the 2 transformFile* files that actually rely on fs so Webpack will complain less.
  • Flowtype on every file in babel-core now except build-external-helpers.
  • We now get useful code snippets in exceptions thrown by babel-template
  • It's a bit easier to follow the transform process now that it's not a chain of class methods.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 2, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5142/

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆒

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Oct 2, 2017
@@ -1,6 +1,5 @@
export default class Hub {
constructor(file, options) {
Copy link
Member

@hzoo hzoo Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it even worth having this file if it's just a file? or just move to src/index.js as export class Hub { }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can revisit, I just didn't want to change any of the actual transform infrastructure.

@loganfsmyth loganfsmyth merged commit cc8109c into babel:master Oct 2, 2017
@loganfsmyth loganfsmyth deleted the file-simplification branch October 2, 2017 21:08
@@ -1,39 +1,42 @@
export File from "./transformation/file";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we continue to export File? I need it in a number of places to create nice error messages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants