Skip to content

Feature/es6 module build #3136

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

Closed
wants to merge 3 commits into from

Conversation

neeharv
Copy link

@neeharv neeharv commented Sep 15, 2017

HACK ALERT. This is an extremely WIP hacky way to do split builds into ES6 and ES5 builds, as described in #3125. I am simply putting this up to spark discussion on the right way to do this.

This works by -

  1. Turning webpack.prod.js into a factory function that takes in target and returns different configs with varying targets
  2. We suffix the module build assets with .es6.js (this can be moved to config easily)
  3. For the ES6 build, we don't include polyfills
  4. We get 2 asset-manifests this way, and hence 2 different service workers that load the relevant built code
  5. Switch over to "uglifyjs-webpack-plugin": "^1.0.0-beta.2" so we can minify the ES6 code
  6. It currently only spits out the ES6 build only in this PR, because the HtmlWebpackPlugin looks at webpack's stats file to generate the variables for the template. I have a local version of HtmlWebpackPlugin that reads the assets of both builds and makes them available for the template, but that is beyond the scope of this and an extremely bad way of doing things

An alternate to this would be to somehow get webpack to accept two different loader configurations for two different entry points, in the same build. AFAIK, this is what @geelen was trying and webpack today doesn't support this. Hence the above approach of running two parallel builds and then using a custom templating job to add the asset names of the ES5 and the ES6 code to the output html.

@viankakrisna
Copy link
Contributor

I think #3103 could be the first step towards this


const getBabelPreset = (babelPresetObj, presetTarget) => {
const clonedBabelPresetObj = Object.create(babelPresetObj);
clonedBabelPresetObj.presets[0][1].targets = presetTarget;
Copy link
Contributor

Choose a reason for hiding this comment

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

changes in #3103 would make this less hacky IMO

Choose a reason for hiding this comment

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

Isn't babel 7.0 babelrc.js a solution to this?

Copy link
Author

Choose a reason for hiding this comment

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

@diligiant Not really, because as I mentioned below, both .babelrcand babelrc.js are required once and then cached. So can't switch things in them based on env variables for eg.

Choose a reason for hiding this comment

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

@neeharv you mean there is no way you could offload the configuration to babelrc.js, invoking it yourself, in order to “preserve” some kind of consistency in regard to how Babel will be usually configured from now on?
(Not sure I make myself understandable ;)

@neeharv
Copy link
Author

neeharv commented Sep 15, 2017 via email

@philipwalton
Copy link

Awesome to see this being implemented! A few issues I noticed:

  1. You're going to need to inline the nomodule fix somewhere in the HTML for Safari 10.x, otherwise it'll execute both files.
  2. You're also going to need to add the {mangle: {safari10: true}} config option to the UglifyJSPlugin() or you'll get syntax errors in Safari due to bugs in its ES2015 variables implementation (which have since been fixed). You can reference how I do it in my boilerplate for an example.

@icopp
Copy link

icopp commented Sep 21, 2017

An alternate to this would be to somehow get webpack to accept two different loader configurations for two different entry points, in the same build.

If you give webpack an array of objects rather than an object, it will run all of them. They're effectively separate builds (even if you're outputting to the same folder), but webpack-dev-server will live update both and so. See their example of doing such.

Also, for my part I'm wondering if .mjs as an extension wouldn't be better.

@neeharv
Copy link
Author

neeharv commented Oct 4, 2017

@icopp true, but then two separate manifests are spat out, which is the problem. We want a combined manifest with two different configurations for two different entry points, because our final HTMLWebpackPlugin will inline filenames from the manifest

@diligiant
Copy link

@icopp both configs are marginally different which makes @neeharv and @philipwalton approach DRY.

@icopp
Copy link

icopp commented Oct 4, 2017

@neeharv There are already two asset manifests being generated with the current version, unless I'm missing something. Using a single config that's an array still has that problem, but it lets you use the webpack dev server, which running things as two separate builds doesn't.

@diligiant You can generate two objects in an array dynamically from a single config and then pass the final result in to Webpack, though.

@diligiant
Copy link

@icopp my bad, I thought you were willing to have 2 webpack.prod.js files (dev and prod are very different so it makes sense to maintain 2 files). That's a great idea.

@neeharv
Copy link
Author

neeharv commented Oct 5, 2017

@icopp good points, I'll switch this over to the array build so we can use dev server. As I commented earlier, the merging of the two manifests is the biggest unsolved problem and what I need help with. I have a version of HTMLWebpackplugin locally that very hackily works with multiple manifests but obviously that is not the ideal way to do this. It'd be best if webpack itself can spit this out, or we use a custom build system for the HTML and not HTMLWebpackplugin

@geelen
Copy link

geelen commented Oct 6, 2017

Hmm, I still think the two webpack builds is not the way to go. The fact that you're trying to merge manifests (or load them both) feels like a warning sign. Is the main blocker to a single webpack build just the babel env caching stuff?

@rayshan
Copy link

rayshan commented Oct 12, 2018

@neeharv thanks for experimenting with this. Can it be closed in favor of #4964?

@dandv
Copy link

dandv commented Nov 6, 2018

@rayshan: #4964 was closed yesterday.

@rayshan
Copy link

rayshan commented Nov 7, 2018

@dandv by mistake, reopened.

@stale
Copy link

stale bot commented Dec 7, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 7, 2018
@stale
Copy link

stale bot commented Dec 12, 2018

This pull request has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue. Thank you for your contribution!

@stale stale bot closed this Dec 12, 2018
@lock lock bot locked and limited conversation to collaborators Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants