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

Quality of life improvements for node usage #301

Conversation

lightsofapollo
Copy link
Contributor

This PR changes how node targets are built:

  • No more uglify (not compatible /w latest node 6 syntax ). Generally I think downstream consumers will only care about browser or will uglify their entire pipeline (which will effectively uglify this).

  • Node specific babel preset (targets node 6 per package.engine)


"This fixes the babel-polyfill included twice" bug I ran into and has a nice side effect of making the node builds much smaller.

I tested : grunt node-qunit, grunt mocha (has one existing failure on master), mocha in browser (for chrome)...

Allows node 6 to run natively as this operation is invalid (at least
under v8).
 - Removes uglify from node targets (uglify _still_ does not understand
   es2015+)
 - Use babel-env-preset to figure out what are the best options for
   target node (6.9 used as package.json engine specifies)
@lightsofapollo
Copy link
Contributor Author

@brettz9 r?

@brettz9
Copy link
Collaborator

brettz9 commented May 3, 2017

Hey, thanks for the changes! I was thinking we could probably dump those minified Node builds (I had been using Node 6 for a while before upgrading too, but maybe I didn't notice as "main" is pointing to the non-minified version, perhaps because of that!).

A few questions/concerns:

  1. Do the babel transform options not themselves handle conversion of class if Node doesn't support the constructs yet? It looks like they should though per http://node.green/#ES2015-functions-class . (I was seeing occasional problems with class and Node for at least one case (as well as Object.entries for that matter), which is why I commented out class DummyDOMException extends Error). It'd be nice if we could keep the more elegant syntax if possible.
  2. Does there really need to be an empty file at "babel-polyfill-after"? Why was the import './babel-polyfill-after'; necessary or was that a relic from testing?
  3. While not a big deal, could you adjust so that babelNodeOptions / babelBrowserOptions derive common options from a single object (possibly derive one from the other), e.g., via Object.assign, and just tweak the necessary portion(s)? Helpful to see quickly by code which config are environment-specific and avoid redundancy...

And yes, good catch on memoryDatabase! If you offer the explanations/make the changes, I'll pull to my own fork and run the W3C tests, etc. too.

@brettz9
Copy link
Collaborator

brettz9 commented May 3, 2017

I've added your commits, rebasing the last one to add babel-preset-env to devDependencies. I've also addressed the above-mentioned issues (with the exception that I didn't look into the necessity of the class changes) and made a few necessary fixes caught by the W3C tests (listed in the commit).

Very helpful to have this, thanks! Btw, though it requires some work, if you can get the W3C tests set up, it would really help future testing. But regardless, it is good to have these changes!

@brettz9 brettz9 closed this May 3, 2017
@lightsofapollo
Copy link
Contributor Author

@brettz9 Thanks for getting this landed so fast! Let me address some of your questions:

Do the babel transform options not themselves handle conversion of class if Node doesn't support the constructs yet? It looks like they should though per http://node.green/#ES2015-functions-class . (I was seeing occasional problems with class and Node for at least one case (as well as Object.entries for that matter), which is why I commented out class DummyDOMException extends Error). It'd be nice if we could keep the more elegant syntax if possible.

babel-env will correctly figure out what is supported by node (in this case class is) but there seem to be some additional constraints when using class (I think those could have been easily fixed by using Object.setPrototypeOf but switching to class seemed easier).

Does there really need to be an empty file at "babel-polyfill-after"? Why was the import './babel-polyfill-after'; necessary or was that a relic from testing?

Hah- This was my mistake... Originally contained:

const glob = typeof global !== 'undefined' ? global : (typeof window !== 'undefined' ? window : self);
glob._babelPolyfill = false; // http://stackoverflow.com/questions/31282702/conflicting-use-of-babel-register

This was in theory to prevent the exact error that prompted me to submit the patch... It may work in a different environment but IMO it's better to remove it (and polyfill in general if you can avoid it).

@brettz9
Copy link
Collaborator

brettz9 commented May 3, 2017

Yeah, thanks. For strict compliance, I actually already added Object.setPrototypeOf (these supposedly poorly-performing calls are handled in setGlobalVars but only if the user opts in, as needed in our W3C interface tests). Yes, class would be easier, but I guess you mean that babel-env wouldn't patch class to fix things for us.

You encountered the duplicate babel-polyfill error even with the check we had in place? Also, do we need the import babel-polyfill command alone in its own file or can we move the command back inline (or is that what has seemingly avoided the duplicate calls to babel-polyfill?)

@lightsofapollo
Copy link
Contributor Author

@brettz9 I suspect the code I referenced above does not actually work... IMO it would be best to avoid using babel-polyfill in libs and let the individual consumers add in the polyfill (though I have seen all different kinds of combinations of this (i.e. using transform-runtime)).

@lightsofapollo
Copy link
Contributor Author

... and yes the new babel-polyfill-before.js could just be removed in favor of the direct babel-polyfill import

@brettz9
Copy link
Collaborator

brettz9 commented May 3, 2017

The code referenced above had been working to avoid babel-polyfill throwing upon encountering another instance and didn't seem to cause any issues, but I do get the rationale for letting downstream users handle. Not sure babel-polyfill still does this, but as I ran all of the browser tests, it looks like we're good now atm, regardless...

Since it is an extra step for people to do it themselves and we're doing the build anyways, since our project is of broad appeal and I'd like to reduce the complications toward entry and getting started, and since I think there are too many appealing cases to keep using the polyfilled prototypes for which browser support may vary for some time (thanks for the info on transform-runtime btw), I hesitate not to include it.

It is admittedly just one additional browser include (which we could instruct users on including), and it would be reassuring to use babel-polyfill in the recommended manner of not browserifying it as well as help anyone knowing the browser supports the features to be able to abandon the extra file size. But at this point, I think the costs of switching may outweigh the benefits.

@brettz9
Copy link
Collaborator

brettz9 commented May 10, 2017

Ok, given another issue (#303) reporting on this (a problem with our removal of a polyfill check for code using babel-polyfill outside of IDB), I've decided users should be able to handle the newly-added README instructions requiring the babel-polyfill file in the browser. Doesn't affect Node but thought I'd mention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants