webpack icon indicating copy to clipboard operation
webpack copied to clipboard

feat: tree shakable output for module library

Open fi3ework opened this issue 1 year ago • 8 comments

What kind of change does this PR introduce?

A prototype PR trying to implement part of #17121. This PR is still rough at present, any modification suggestions and implementation ideas are welcomed. 🙌🙌🙌.

We're trying to bundle high-quality output NPM libraries with Webpack / Rspack in ESM format. For now, all exported identifiers are defined and re-declarad from __webpack_exports__, which makes live-binding and tree-shaking broken when consuming the ESM package (as described in https://github.com/webpack/webpack/issues/2933#issuecomment-874330847), and that's what this PR trying to tackle.

This PR tries to only fix the issue in a limited scene when simultaneously satisfies the following conditions:

  1. output.library.type is "module": The premise of this problem is ESM library.
  2. optimization.concatenateModules is true: the modules need to be flatten in the same top-level scope.
  3. Requires only one chunk in the EntryPoint chunkGroup, only then the concatenatedModule in the top level, which means optimization.runtimeChunk is unapplicable.

I created a gist to demonstrate how this PR changes the output, the file to be bundled are some regular ESM inputs and there's also a CJS input.

The change is quite straightforward:

  1. Adding a bail hook in lib/optimize/ConcatenatedModule.js and skip defining properties on __webpack_exports__ when the hook returns true.
  2. If defining properties on __webpack_exports__ is skipped, the final exports name from ConcatenatedModule will be saved in buildMeta.exportsFinalName to let lib/library/ModuleLibraryPlugin.js consumsing it afterwards.
  3. lib/library/ModuleLibraryPlugin.js will tap the hook foreamentioned and return true to instruct skipping the defining.
  4. If the defining is skipping, in renderStartup of lib/library/ModuleLibraryPlugin.js will try to read buildMeta.exportsFinalName from the module and complete the module export with the finalName and exportInfo.name directly without leveraging __webpack_exports__.

Some unperfect implements and concerns:

  1. When output.library.type is module, skip IIFE wrap in lib/javascript/JavascriptModulesPlugin.js directly.
  2. Requires only one chunk in EntryPoint chunksGroup, single chunk could meet common usage of library bundling.

Did you add tests for your changes?

Yes, consuming a ESM library and the unused exports are tree shaked.

Does this PR introduce a breaking change?

Somehow yes, that would makes live-binding and tree-shaking available, and also skipped IIFE wrapping in some condition. Maybe we could introduce a new library.type if we decided to move forward.

What needs to be documented once your changes are merged?

Yes, depends on the final implementation.

fi3ework avatar Apr 02 '24 18:04 fi3ework

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: fi3ework (f6b9af6ed0bac30ab688f5801ae4b1d98c521f6a, 03aab9478a578abca0b463656ba408cd66a5b68c, 86336d9894f04d08c3160bef6ad4d20d9666fc6b, 475684c9084d6abb80bd8bc3570269addd5f5e62, c54d4a4b1f676ca0a1aea8340f4a063c99f1fef2, 5cff7bd0e71a963635f909233ea808ed95206b70)

For maintainers only:

  • [ ] This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • [ ] This needs to be backported to webpack 4 (issue will be created when merged)

webpack-bot avatar Apr 02 '24 18:04 webpack-bot

Requires only one chunk in EntryPoint chunksGroup, single chunk could meet common usage of library bundling.

Can you descibe this limitation better and any ideas?

Somehow yes, that would makes live-binding and tree-shaking available, and also skipped IIFE wrapping in some condition. Maybe we could introduce a new library.type if we decided to move forward.

Technically es modules output is experiment and it is fine to make such changes, but yeah some developers can lead on the current output, maybe modern-module (maybe better name? :smile: ) is good idea (only import, import.meta.url, live-binding, tree-shaking, etc), and for webpack6 we will use only it

alexander-akait avatar Apr 02 '24 18:04 alexander-akait

Can you descibe this limitation better and any ideas?

This restriction comes from the test case regression. When there is more than one chunk in a chunkGroup (such as using optimization.runtimeChunk), it will cause the concatenated module to be wrapped in a closure, which makes it impossible to use only the ESM export syntax directly. The original way of using __webpack_exports__ is fine.

Technically es modules output is experiment and it is fine to make such changes, but yeah some developers can lead on the current output, maybe modern-module (maybe better name? 😄 ) is good idea (only import, import.meta.url, live-binding, tree-shaking, etc), and for webpack6 we will use only it

Yeah, we could adding a new type and implements features gradually in different PRs.

fi3ework avatar Apr 02 '24 18:04 fi3ework

@alexander-akait

Can you describe this limitation better and any ideas?

tl;dr

the modern bundled library idea is built on top of a well-worked concatenate module in top-level, but the concatenate module doesn't always work as expected (we got bailout situations). so we had to add some limitations to ensure consuming a concatenated module as expected. But there are some cases we could not handle for far (described below).

details

As mentioned above, optimization.concatenateModules plays a core role, we will lose tree-shaking and live-binding without it. So we need to ensure the concatenated modules could directly export its identifiers in the top-level. Therefore, in modern-module type, we will force opening optimization.concatenateModules first.

The problem is there will be an IIFE wrapper for the concatenated module in some specific situations. Currently, there are four situations will lead to IIFE wrapper.

// https://github.com/webpack/webpack/blob/main/lib/javascript/JavascriptModulesPlugin.js#L837-L847
let iife = innerStrict
	? "it need to be in strict mode."
	: inlinedModules.size > 1
		? // TODO check globals and top-level declarations of other entries and chunk modules
			// to make a better decision
			"it need to be isolated against other entry modules."
		: chunkModules
			? "it need to be isolated against other modules in the chunk."
			: exports && !webpackExports
				? `it uses a non-standard name for the exports (${m.exportsArgument}).`
				: hooks.embedInRuntimeBailout.call(m, renderContext);

For each situation:

  1. innerStrict: to output in ESM type, this will always be false as renderContext.strictMode is always true.

  2. inlinedModules.size > 1:

    1. Modern module libraries usually use one single file as the bundle entry, which means entry options like entry: myLib: ['./index.js', './other.js'] are out of context. This will result in multiple concatenated modules. Each of them will be converted to the inlinedModules and requiring wrapped in the IIFE to avoid conflict against others, then the concatenate module broken. So when we encounter this option, we throw an error directly.

    2. Like 1, we throw an error directly for multiple chunks in a single EntryPoint, considering it's off the context.

  3. 🔴 chunkModules: this would happen when an ESM module requires an CJS module (and other concatenate modules bailout cases), the CJS module will bail out in the module concatenation calculation and output a separated module. The related test case is https://github.com/webpack/webpack/blob/main/test/cases/entry-inline/no-var-leak/index.js. AFAICT, when encountering this case, we can't emit a concatenated module without IIFE wrapper and keep it from leaking to other modules. esbuild handles this well (playground) as it takes count of CJS module in scope hoisting calculation.

  4. exports && !webpackExports: the m.exportsArgument is still RuntimeGlobals.exports so this will always be false.

ideas

I still believe that concatenated module is the fundamental foundation principle of this feature. The most ideal situation is that we always can get a concatenated module comprising all modules. Starting from this point, we could add an option for the concatenated module strategy that can tolerate more situations only in ESM format (which means tolerates all the bailout situations), the trade-off might be the correctness in some edge case like esbuild. This approach is also more like other library builders (esbuild/rollup). I am not sure about the feasibility of this idea and would appreciate your feedback. 🙌🙌🙌

Technically es modules output is experiment and it is fine to make such changes, but yeah some developers can lead on the current output, maybe modern-module (maybe better name? 😄 ) is good idea (only import, import.meta.url, live-binding, tree-shaking, etc), and for webpack6 we will use only it

modern-module seems to be a good name IMO.

fi3ework avatar Apr 08 '24 18:04 fi3ework

@fi3ework Sound good for me, can we convert it in checkboxes/tasks to track progress (also we already solved some cases)

alexander-akait avatar May 07 '24 14:05 alexander-akait

@fi3ework Sound good for me, can we convert it in checkboxes/tasks to track progress (also we already solved some cases)

@alexander-akait

As discussed above, the tree shakable output this PR trying to address is a subset of #17121 target. We're going to add a modern-module library type to progressively landing all the feature #17121 lists. Here are the tasks remain for this PR.

  • [x] Type re-export will still be exported in JS when there's no import type or export type hint. This will break the syntax in the new way while works fine in the old way. I will try to raise another PR to tackle this if it can be resolved is webpack side first, otherwise I will make a workaround in this PR.
    / before
    ar __webpack_exports__ = {};
    ar actions_namespaceObject = {};
    _webpack_require__.r(actions_namespaceObject);
    _webpack_require__.d(__webpack_exports__, {
     Action: () => (/* reexport */ actions_namespaceObject.Action),
    )
    ar __webpack_exports__Action = __webpack_exports__.Action;
    xport { __webpack_exports__Action as Action } 
    
    / after
    ar actions_namespaceObject = {};
    _webpack_require__.r(actions_namespaceObject);
    xport { actions_namespaceObject.Action as Action }
    
  • [x] Change the modification to modern-module type as suggested above.
    • [x] documentation: IMO, we do not need to document the modern-module type at this time. We can open the modern-module type to user to get more feedback once we have confirmed that the necessary features in #17121 are met. Before that, we can keep developing the modern-module type internally.
  • [x] IIFE wrapper: we have addressed the it need to be isolated against other modules in the chunk. condition which will be raised when a CJS file is included in bundling. The other two conditions as discussed above are out of context for ESM library output so far (but we still can make improve to eliminate them :-P).

That's the TODO remains AFAICR, I'll tackle them in these days. Feel free to add any things I missed. Thanks! 🙌

fi3ework avatar May 09 '24 08:05 fi3ework

Updated the PR and clean the tasks remain. There are some known configurations will break the ESM output, however they won't be used in most actual uses, and I think it's acceptable.

  1. When the entry has multiple modules, be like entry: { ['./lib1.mjs', './lib2.mjs'] }, the final export of output will use lib2's export while lib1's will be ignored as we're only using the last inlinedModule for now (source: https://github.com/webpack/webpack/blob/main/lib/javascript/JavascriptModulesPlugin.js#L923). As discussed above, user rarely use an entry with multiple modules, so I think we could keep ignoring this as we already do. This will break both in the original module type and the new modern-module type.
  2. Using splitChunks / runtimeChunk will break the inlinedModule premise, but this is rarely used in library building.

~~And removed writing on buildeMeta to convey the exports definition as we have assumed that not IIFE will wrap the inlined module.~~ We still need to use buildMeta to make sure the exports names binds on each module, otherwise it will break in bundleless mode.

Ready to be reviewed, any suggestions are kindly welcomed.

fi3ework avatar May 13 '24 18:05 fi3ework

@fi3ework Sorry for small delay, what is current status? Should be make more PRs or refactor something?

alexander-akait avatar Jun 07 '24 15:06 alexander-akait

Also we need to fix tests and rebase

alexander-akait avatar Jun 07 '24 15:06 alexander-akait

Also we we will need update our docs and show different between module and modern-module, but let's do it after it will be merged

alexander-akait avatar Jun 07 '24 15:06 alexander-akait

@fi3ework Can you update description about a new value for other users and our docs and we can merge, thank you

alexander-akait avatar Jun 19 '24 17:06 alexander-akait

When the entry has multiple modules, be like entry: { ['./lib1.mjs', './lib2.mjs'] }, the final export of output will use lib2's export while lib1's will be ignored as we're only using the last inlinedModule for now (source: https://github.com/webpack/webpack/blob/main/lib/javascript/JavascriptModulesPlugin.js#L923). As discussed above, user rarely use an entry with multiple modules, so I think we could keep ignoring this as we already do. This will break both in the original module type and the new modern-module type.

there is an issue - https://github.com/webpack/webpack/issues/15936, so let's fix it for module and modern-module later

Using splitChunks / runtimeChunk will break the inlinedModule premise, but this is rarely used in library building.

We have issues for this too

alexander-akait avatar Jun 19 '24 17:06 alexander-akait

If you want help we can focus on https://github.com/webpack/webpack/issues/15936 after merge (PR and help welcome :smile: )

alexander-akait avatar Jun 19 '24 17:06 alexander-akait

Module output is still experimental so we can change this behavior in any time

alexander-akait avatar Jun 19 '24 18:06 alexander-akait

Thank you

alexander-akait avatar Jun 19 '24 18:06 alexander-akait

Any other things are in our roadmap for this?

alexander-akait avatar Jun 19 '24 18:06 alexander-akait

@alexander-akait Sorry for the delayed response, I took a short vacation a while back. Now I'll continue the work.

Also we we will need update our docs and show different between module and modern-module, but let's do it after it will be merged @fi3ework Can you update description about a new value for other users and our docs and we can merge, thank you

I'd prefer to add more test cases that might break in the modern-module. After we've addressed known issue, we could update the docs.

By the way, I'm not sure when do we reach the point where we can open it up to users. As per https://github.com/webpack/webpack/issues/17121, there are still a lot of TODOs ("Build output supports tree-shaking" is resolved as of now). Any suggestion for how we release modern-module to users?

there is an issue - https://github.com/webpack/webpack/issues/15936, so let's fix it for module and modern-module later

If you want help we can focus on https://github.com/webpack/webpack/issues/15936 after merge (PR and help welcome 😄 )

Sure, I could give it a try. IMO, it's not commonly used in building a NPM library (any usage in production we could inspect there?). Are tasks remains on https://github.com/webpack/webpack/issues/17121 in higher priority?

Using splitChunks / runtimeChunk will break the inlinedModule premise, but this is rarely used in library building.

It's an issue. But might this be in lower priority than tasks remain in https://github.com/webpack/webpack/issues/17121? This is not common when bundling libraries.

fi3ework avatar Jun 24 '24 06:06 fi3ework

Are tasks remains on https://github.com/webpack/webpack/issues/17121 in higher priority?

Yeah, of couse, any of these tasks have higher priority

alexander-akait avatar Jul 10 '24 17:07 alexander-akait