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

Add a 'lazy' options to modules-commonjs #6952

Merged
merged 2 commits into from Dec 18, 2017

Conversation

Projects
None yet
8 participants
@loganfsmyth
Member

loganfsmyth commented Dec 1, 2017

Q                       A
Fixed Issues?
Patch: Bug Fix? N
Major: Breaking Change? N
Minor: New Feature? Y
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Not inspired by the code from https://github.com/zertosh/babel-plugin-transform-inline-imports-commonjs itself, but certainly inspired by talking to @zertosh about it in the past, and now feeling like we might want to use it for Babel's own codebase. It's pretty trivial to include in our helper-module-transforms so I figured why not.

Allow import bindings to lazy-initialize themselves the first time they are accessed, instead of up front at file load time.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Dec 1, 2017

Collaborator

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

Collaborator

babel-bot commented Dec 1, 2017

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

typeof lazy !== "function" &&
(!Array.isArray(lazy) || !lazy.every(item => typeof item === "string"))
) {
throw new Error(`.lazy must be a boolean, array of strings, or a function`);

This comment has been minimized.

@danez

danez Dec 2, 2017

Member

Is the . at the beginning intentional?

@danez

danez Dec 2, 2017

Member

Is the . at the beginning intentional?

This comment has been minimized.

@xtuc

xtuc Dec 2, 2017

Member

It's because it's a key in the configuration.

@xtuc

xtuc Dec 2, 2017

Member

It's because it's a key in the configuration.

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

I guess we don't really have a good naming scheme for these errors. I just included it since it is an object property. I can change it.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

I guess we don't really have a good naming scheme for these errors. I just included it since it is an object property. I can change it.

This comment has been minimized.

@xtuc

xtuc Dec 2, 2017

Member

At least this is consistent with the other error messages used in Babel but it's not super clear to me. What about: "the key 'lazy' in the Babel configuration must be a boolean". Can be done in a second PR. That reminds my that I still have a PR to add code frames to configuration issues. I can continue my work, what do you think?

@xtuc

xtuc Dec 2, 2017

Member

At least this is consistent with the other error messages used in Babel but it's not super clear to me. What about: "the key 'lazy' in the Babel configuration must be a boolean". Can be done in a second PR. That reminds my that I still have a PR to add code frames to configuration issues. I can continue my work, what do you think?

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 18, 2017

Member

What about: "the key 'lazy' in the Babel configuration must be a boolean".

I don't think it's much clearer than what I have here, but I do think we could do better in general with work focused on making the error messages give better feedback.

That reminds my that I still have a PR to add code frames to configuration issues. I can continue my work, what do you think?

Not sure I follow. What code frames could we show for this case? Do you mean showing information about the config file itself?

@loganfsmyth

loganfsmyth Dec 18, 2017

Member

What about: "the key 'lazy' in the Babel configuration must be a boolean".

I don't think it's much clearer than what I have here, but I do think we could do better in general with work focused on making the error messages give better feedback.

That reminds my that I still have a PR to add code frames to configuration issues. I can continue my work, what do you think?

Not sure I follow. What code frames could we show for this case? Do you mean showing information about the config file itself?

@@ -132,14 +143,26 @@ export default function(api, options) {
let header;
if (isSideEffectImport(metadata)) {
if (metadata.lazy) throw new Error("Assertion failure");

This comment has been minimized.

@danez

danez Dec 2, 2017

Member

I guess this should never really happen, but maybe we could make the error a little bit more descriptive?

@danez

danez Dec 2, 2017

Member

I guess this should never really happen, but maybe we could make the error a little bit more descriptive?

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

For assertions I don't usually bother. I'd just assert(!metadata.lazy) but usually that isn't enough to make Flowtype happy so I've kind of got into the habit of doing it like this. I really just threw this in for us.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

For assertions I don't usually bother. I'd just assert(!metadata.lazy) but usually that isn't enough to make Flowtype happy so I've kind of got into the habit of doing it like this. I really just threw this in for us.

Local paths are much more likely to have circular dependencies, which may break if loaded lazily,
so they are not lazy by default, whereas dependencies between independent modules are rarely cyclical.
* `Array<string>` - Lazy-initialize all imports with source matching one of the given strings.

This comment has been minimized.

@danez

danez Dec 2, 2017

Member

do we need Array<string>, do you think it is a common case? I guess boolean | (string) => boolean would capture this case too. I was also thinking about more cases like "enable really everything" or "enable everything with a blacklist", but that can all be handled with the callback.

@danez

danez Dec 2, 2017

Member

do we need Array<string>, do you think it is a common case? I guess boolean | (string) => boolean would capture this case too. I was also thinking about more cases like "enable really everything" or "enable everything with a blacklist", but that can all be handled with the callback.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Dec 2, 2017

Member

The disadvantage of the callback is that it forces the user to use .babelrc.js instead of .babelrc

@nicolo-ribaudo

nicolo-ribaudo Dec 2, 2017

Member

The disadvantage of the callback is that it forces the user to use .babelrc.js instead of .babelrc

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

Yeah the .babelrc/package.json#babel case was the reason I added the array. Seems like a whitelist was easy enough to support that I'd just throw it in.

@loganfsmyth

loganfsmyth Dec 2, 2017

Member

Yeah the .babelrc/package.json#babel case was the reason I added the array. Seems like a whitelist was easy enough to support that I'd just throw it in.

@xtuc

Awesome, that's really nice!

typeof lazy !== "function" &&
(!Array.isArray(lazy) || !lazy.every(item => typeof item === "string"))
) {
throw new Error(`.lazy must be a boolean, array of strings, or a function`);

This comment has been minimized.

@xtuc

xtuc Dec 2, 2017

Member

It's because it's a key in the configuration.

@xtuc

xtuc Dec 2, 2017

Member

It's because it's a key in the configuration.

@zertosh

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Dec 3, 2017

Member

Super cool! Question, why did you decide to go with always calling the memoized require rather than conditionally getting the previously stored require value? For compat with modules whose value is falsey ? If always calling the memoized require's perf is on-par or better then I'll deprecate babel-plugin-transform-inline-imports-commonjs

Member

zertosh commented Dec 3, 2017

Super cool! Question, why did you decide to go with always calling the memoized require rather than conditionally getting the previously stored require value? For compat with modules whose value is falsey ? If always calling the memoized require's perf is on-par or better then I'll deprecate babel-plugin-transform-inline-imports-commonjs

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Dec 3, 2017

Member

@zertosh I honestly didn't think on it too much. If anyone wants to do some benchmarks to compare, and your approach of having a separate variable is noticeably faster, we can certainly do it. I went this route because it seemed like it'd be easier for people debugging code to just have a single name to reference, and because it seemed like this approach would be pretty easy for engines to inline, since the function never changes and is statically guaranteed to always return the same value.

Member

loganfsmyth commented Dec 3, 2017

@zertosh I honestly didn't think on it too much. If anyone wants to do some benchmarks to compare, and your approach of having a separate variable is noticeably faster, we can certainly do it. I went this route because it seemed like it'd be easier for people debugging code to just have a single name to reference, and because it seemed like this approach would be pretty easy for engines to inline, since the function never changes and is statically guaranteed to always return the same value.

@nicolo-ribaudo

Really nice feature 🚀

@loganfsmyth loganfsmyth merged commit 44ea943 into babel:master Dec 18, 2017

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.21% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@loganfsmyth loganfsmyth deleted the loganfsmyth:commonjs-lazy branch Dec 18, 2017

@jamesreggio

This comment has been minimized.

Show comment
Hide comment
@jamesreggio

jamesreggio Feb 19, 2018

Contributor

@zertosh @loganfsmyth — I did some empirical benchmarking of both in my React Native app and witnessed the new lazy option to be 24% faster than babel-plugin-transform-inline-imports-commonjs (with a variance of 5%).

It's possible that other changes in the Babel 7 version of the mainline commonjs plugin are to account for these improvements, but I'd say — from my limited perspective — that it's safe to sunset babel-plugin-transform-inline-imports-commonjs.

Thank you both for this feature. It's made a huge improvement in my cold-start time.

(Addendum: if anybody would like to try using babel-plugin-transform-inline-imports-commonjs with Babel 7, pull my branch here.)

Contributor

jamesreggio commented Feb 19, 2018

@zertosh @loganfsmyth — I did some empirical benchmarking of both in my React Native app and witnessed the new lazy option to be 24% faster than babel-plugin-transform-inline-imports-commonjs (with a variance of 5%).

It's possible that other changes in the Babel 7 version of the mainline commonjs plugin are to account for these improvements, but I'd say — from my limited perspective — that it's safe to sunset babel-plugin-transform-inline-imports-commonjs.

Thank you both for this feature. It's made a huge improvement in my cold-start time.

(Addendum: if anybody would like to try using babel-plugin-transform-inline-imports-commonjs with Babel 7, pull my branch here.)

@jamesreggio jamesreggio referenced this pull request Feb 19, 2018

Closed

WIP: Support Babel 7 #11

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Feb 19, 2018

Member

@jamesreggio Happy to hear it. Beware there's one known bug: #7176

Member

loganfsmyth commented Feb 19, 2018

@jamesreggio Happy to hear it. Beware there's one known bug: #7176

@zertosh

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Feb 19, 2018

Member

@jamesreggio, interesting - I expected some difference one-way or the other, but not that stark. Curious to see this is a way I can repro (just for my know education). I'll deprecate babel-plugin-transform-inline-imports-commonjs as soon as Babel 7.0 is stable.

Member

zertosh commented Feb 19, 2018

@jamesreggio, interesting - I expected some difference one-way or the other, but not that stark. Curious to see this is a way I can repro (just for my know education). I'll deprecate babel-plugin-transform-inline-imports-commonjs as soon as Babel 7.0 is stable.

@jamesreggio

This comment has been minimized.

Show comment
Hide comment
@jamesreggio

jamesreggio Feb 20, 2018

Contributor

@zertosh — I was surprised too. I thought your use of a local variable would be more performant. I wish I could supply a repro to you, but I'm measuring real-user scenarios in our closed-source app, and I don't have time to build a separate benchmark suite.

Contributor

jamesreggio commented Feb 20, 2018

@zertosh — I was surprised too. I thought your use of a local variable would be more performant. I wish I could supply a repro to you, but I'm measuring real-user scenarios in our closed-source app, and I don't have time to build a separate benchmark suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment