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

Remove targets from library preset to force es2015 target #1322

Merged
merged 3 commits into from
Mar 24, 2019

Conversation

eliperelman
Copy link
Member

Fixes #1279.

Just an initial implementation; happy to change if this seems incorrect.

@eliperelman eliperelman added this to the v9 milestone Mar 13, 2019
@eliperelman eliperelman self-assigned this Mar 13, 2019
@eliperelman eliperelman requested a review from a team March 13, 2019 21:26
packages/library/index.js Outdated Show resolved Hide resolved
@eliperelman eliperelman requested a review from a team March 14, 2019 20:31
@eliperelman
Copy link
Member Author

Re-tagging for review to see if we can get approval or changes.

@edmorley
Copy link
Member

I'm a little hazy on this area, since it's been a while since I thought about #1279, but in that issue I believe the main problem was that @neutrinojs/library could output code that was broken unless people also imported a polyfill, but neither imported it for them, nor made it clear that they may need to so themselves.

The addition of the readme note in this PR address the final point of that, however the change to force all transforms seems to not be required, or at least orthogonal, given that the target of IE9 already activated most transforms, and that's why the pollyfill was required in the first place?

It seems that we need to decide the following:

  1. Should the default target for @neutrinojs/library be "~all browsers", "modernish browsers", or both (using a combination of main and module in the resultant package.json)?
  2. If for (1) we decide "~all browsers" or "both", is it better to implement this using a target of "IE9" or using an empty target and/or forceAllTransforms: true? (Is the latter more confusing for people who want to target only "modern browsers" and so are expecting to set a target?)
  3. If for (1) we decide "~all browsers" or "both", is there a viable way to import the polyfill automatically (perhaps things have changed with Babel 7?) or should we just document? (And even if we just document, perhaps the create-project project should include the polyfill with a suitable comment saying to remove if not needed?)

Re the PR implementation as-is, I think the library readme's "Preset Options" section would also need updating for the new default options.

@eliperelman
Copy link
Member Author

Found this in the Babel docs for env:

By default, it has the same behavior as previous presets to compile ES2015+ to ES5.

By not specifying any targets, it will compile to ES5 by default. I think that means we don't need the force transformation.

We unfortunately can't output 2 builds unless there are 2 compilations, and this is controlled by the user's webpack.config.js.

For the polyfill, we can prompt the user for it during create-project installation if you think that is a good path. I am still cautious to go down this route.

@edmorley
Copy link
Member

We unfortunately can't output 2 builds unless there are 2 compilations, and this is controlled by the user's webpack.config.js.

Perhaps .webpack() could return an array? (Either all the time, or just for the library preset)

@eliperelman
Copy link
Member Author

It may be possible, but we would need a way to pass the preset twice with a different variation:

module.exports = neutrino().webpack();
// Should return [libraryWithMain, libraryWithModuleAndWithoutClean]

@eliperelman
Copy link
Member Author

I think we should explore better multiple configuration experience in a feature branch after v9. I may have some ideas, but I don't think it should block this.

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.

Should we also make it easier to override targets? (Like is possible with the web preset)
At the moment doing so requires quite a bit of boilerplate (see example in #1279 (comment)).

@eliperelman
Copy link
Member Author

@edmorley I added targets as a top-level option for this preset.

@edmorley
Copy link
Member

Looks good :-)

@eliperelman eliperelman merged commit 434947e into neutrinojs:master Mar 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants