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

feat: tree shakable output for module library #18272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fi3ework
Copy link
Contributor

@fi3ework fi3ework commented Apr 2, 2024

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 #2933 (comment)), 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.

Copy link

linux-foundation-easycla bot commented Apr 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@webpack-bot
Copy link
Contributor

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)

@alexander-akait
Copy link
Member

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? 😄 ) is good idea (only import, import.meta.url, live-binding, tree-shaking, etc), and for webpack6 we will use only it

@fi3ework
Copy link
Contributor Author

fi3ework commented Apr 2, 2024

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
Copy link
Contributor Author

fi3ework commented Apr 8, 2024

@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.

@alexander-akait
Copy link
Member

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

@fi3ework
Copy link
Contributor Author

fi3ework commented May 9, 2024

@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.

  • 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
    var __webpack_exports__ = {};
    var actions_namespaceObject = {};
    __webpack_require__.r(actions_namespaceObject);
    __webpack_require__.d(__webpack_exports__, {
      Action: () => (/* reexport */ actions_namespaceObject.Action),
    })
    var __webpack_exports__Action = __webpack_exports__.Action;
    export { __webpack_exports__Action as Action } 
      
    // after
    var actions_namespaceObject = {};
    __webpack_require__.r(actions_namespaceObject);
    export { actions_namespaceObject.Action as Action }
  • Change the modification to modern-module type as suggested above.
    • 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 ESM Module Output #17121 are met. Before that, we can keep developing the modern-module type internally.
  • 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! 🙌

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

Successfully merging this pull request may close these issues.

None yet

3 participants