Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Change useBuiltIns to usage #1484

Merged
merged 8 commits into from
Nov 5, 2019

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Nov 4, 2019

Resolves #1470

@edmorley edmorley added this to the Neutrino 9 milestone Nov 4, 2019
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for the PR :-)

packages/library/README.md Outdated Show resolved Hide resolved
docs/migration-guide.md Outdated Show resolved Hide resolved
@timkelty
Copy link
Contributor Author

timkelty commented Nov 4, 2019

@edmorley do you think each preset should have a "If you need polyfills in your code…" callout?

Not sure why currently only Library and React have them currently…

@edmorley edmorley mentioned this pull request Nov 4, 2019
10 tasks
@edmorley
Copy link
Member

edmorley commented Nov 4, 2019

Yeah think adding to all of them makes sense (or at least all of the web related ones; less sure about Node)

@timkelty
Copy link
Contributor Author

timkelty commented Nov 5, 2019

Yeah think adding to all of them makes sense (or at least all of the web related ones; less sure about Node)

I added the note to everything that used useBuiltIns: usage, which included Node.

@edmorley
Copy link
Member

edmorley commented Nov 5, 2019

Remove note from react

I think we should still include the note for the React preset. I think maybe you saw the useBuiltIns: true mention in React and so why the docs entry was removed for that? If so, the useBuiltIns: true is actually a different setting with the same name - it's passed to the React babel preset (see docs here) rather than preset-env.

Perhaps a comment next to it emphasising it's different would be useful, since it confused me yesterday too when reviewing the PR?

@timkelty
Copy link
Contributor Author

timkelty commented Nov 5, 2019

@edmorley with the eagle eye! Good catch. 🦅 👁

@timkelty timkelty force-pushed the babel-preset-useBuiltIns-usage branch from d471ab8 to 6e000d5 Compare November 5, 2019 15:02
@edmorley
Copy link
Member

edmorley commented Nov 5, 2019

Perhaps a comment next to it emphasising it's different would be useful, since it confused me yesterday too when reviewing the PR?

I'm really sorry I meant as a code comment in packages/react/index.js rather than in the README. Though happy to go with whatever you think is best :-)

@timkelty
Copy link
Contributor Author

timkelty commented Nov 5, 2019

@edmorley yeah I thought about that after I pushed…one sec. Think I should keep both?

@edmorley
Copy link
Member

edmorley commented Nov 5, 2019

@timkelty Babel 7.7.0 (released in last day or so) added a new useSpread option to the React preset (only) that supersedes the useBuiltins React option, so I'll likely be replacing the option once I've read up more.

Also even if we don't switch to useSpread, users will rarely need to change the React preset option, since whatever the React plugins output gets transpiled after - ie: using built-ins for the React output is desirable either way, so something we perhaps don't need to worry users about.

@timkelty timkelty merged commit af0b36b into neutrinojs:master Nov 5, 2019
@timkelty timkelty deleted the babel-preset-useBuiltIns-usage branch November 5, 2019 17:20
@edmorley
Copy link
Member

edmorley commented Nov 5, 2019

Thank you :-)

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

Successfully merging this pull request may close these issues.

Make it easier to use useBuiltIns: 'usage'
2 participants