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

Default to `ast:false` and do less work when loading core #7436

Merged
merged 5 commits into from Feb 28, 2018

Conversation

@loganfsmyth
Copy link
Member

loganfsmyth commented Feb 27, 2018

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change? Y, ast now defaults to false
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT
@@ -3,7 +3,6 @@
import path from "path";
import * as context from "../index";
import Plugin from "./plugin";
import merge from "lodash/merge";

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 27, 2018

Author Member

Once we make dependencies lazy-load, not loading lodash during anywhere in src/config makes a big difference.

@@ -64,7 +64,7 @@ export function runSync(
return {
metadata: file.metadata,
options: opts,
ast: opts.ast !== false ? file.ast : null,

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 27, 2018

Author Member

Most callers of Babel don't use this and it is a huge structure. Not including it in the output means when we land caching logic in core one day, we'll have less data to cache by default.

This comment has been minimized.

Copy link
@Sunbridger

helpers.jsx = defineHelper(`
helpers.jsx = () => template.program.ast`

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 27, 2018

Author Member

This just avoids parsing the helper snippets until the first time they are used.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Feb 27, 2018

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

}
}

function mergeObject<T: {}>(target: T, source: T) {

This comment has been minimized.

Copy link
@Andarist

Andarist Feb 27, 2018

Member

maybe we could figure out some better name for this one? i had to look into the implementation to see how it is different from Object.assign, maybe just smth like safeMerge?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 27, 2018

Author Member

How about mergeDefaults since that seems to be the common name for this operation?

This comment has been minimized.

Copy link
@Andarist

Andarist Feb 27, 2018

Member

Sounds good to me 😃

const defineHelper = template.program({ placeholderPattern: false });

helpers.typeof = defineHelper(`
helpers.typeof = () => template.program.ast`

This comment has been minimized.

Copy link
@Andarist

Andarist Feb 27, 2018

Member

was removing placeholderPattern for those helper templates deliberate?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 27, 2018

Author Member

Yeah, the .ast variant already returns the AST unchanged, so there aren't any placeholders to bother searching for.

This comment has been minimized.

Copy link
@Andarist

Andarist Feb 27, 2018

Member

Cool, good to know. Thanks for clarifying

@loganfsmyth loganfsmyth merged commit bf8b252 into babel:master Feb 28, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.32% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth loganfsmyth deleted the loganfsmyth:less-upfront-work branch Feb 28, 2018
This was referenced Mar 15, 2018
@hzoo hzoo mentioned this pull request Mar 15, 2018
3 of 6 tasks complete
loganfsmyth referenced this pull request in facebook/react-native Apr 19, 2018
Summary:
BREAKING CHANGE
This change upgrades the React Native build pipeline from Babel 6 to Babel 7

If you use a `.babelrc` then you'll need to update it to Babel 7 (note that some plugins are no longer relevant, some plugins are automatically upgraded, and some will need some manual love).

Note that you may also need to upgrade your dev env, tests etc, to make sure they work with Babel 7.

Reviewed By: mjesun

Differential Revision: D7591303

fbshipit-source-id: 29cef21f6466633a9c366d1f3c0d3cf874c714db
@ValeraS ValeraS mentioned this pull request Oct 31, 2018
@lock lock bot added the outdated label Oct 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.