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: support named exports with any characters #1549

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

laverdet
Copy link
Contributor

@laverdet laverdet commented Oct 24, 2023

The limitation of only allowing camelCase named exports fails to take into account the full capabilities of module exports. In fact, you can export any name you want with export { local as "string literal" }.

This removes the limitation and allows you to set
exportLocalsConvention back to asIs when using the namedExports option.

Relevant syntax documentation can be found here:
https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export#syntax

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This allows you to set exportLocalsConvention back to any valid option when namedExports is set (which changes the option to "camelCase", and restricts you from changing it). Exported names do not have the same restrictions as source identifiers [since tc39/ecma262#2154] so there is no need for this restriction anymore.

Named exports resolve the Terser bailout condition described in webpack/webpack#17626 but reduce "grepability" of your code. By exporting them "as is" you can more easily find all places which reference a given class name in your project.

Breaking Changes

Requires support for export { local as "string literal" } syntax which is not supported until nodejs v16.x.

This breaks css-modules-typescript-loader since that module uses regular expressions to parse the intermediary generated modules. Any other projects with the same deranged behavior may also break, but by that logic any change is a breaking change.

Additional Info

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@laverdet
Copy link
Contributor Author

This is part of a series of pull requests which adds better ecosystem support for exporting original, non-mangled, CSS names as named exports.

#1549
webpack-contrib/mini-css-extract-plugin#1057
seek-oss/css-modules-typescript-loader#50

Due to the optimization bailout condition [webpack/webpack#17626] with JSON default exports consumed by hoistable functions, original non-minified CSS module class names can be found in the minified source code.

@alexander-akait
Copy link
Member

Can you accept CLA?

@laverdet
Copy link
Contributor Author

@alexander-akait - thanks, CLA accepted

Also please note this change may require a semver major increment since it requires nodejs v16.x. Since a breaking semver increment is required anyway it may make sense to remove the behavior where exportLocalsConvention changes automatically if you set namedExports.

@alexander-akait
Copy link
Member

Some developers wants to keep the old output due migration reasons, that is why we need an option for this, also please don't modify existing tests, please add a new

@laverdet
Copy link
Contributor Author

I wasn't recommending removing the option entirely, I was recommending changing the default. Right now if you specify namedExports then exportLocalsConvention will be automatically set to "camelCase". This made sense in the past because some CSS names were not representable in ECMAScript modules. With the newer syntax the behavior where exportLocalsConvention is set automatically no longer makes sense, and only complicates configuration. If a user wants to set it back to "camelCase" then they can do that.

Also I didn't modify any tests, the snapshots were modified as a result of the new feature. The test snapshots will change if there is a modification to the generated JavaScript or CSS.

@laverdet
Copy link
Contributor Author

laverdet commented Jan 2, 2024

@alexander-akait I pushed the proposed modifications. This drops support for nodejs v12 and v14 which do not export { foo as "string" }. Additionally exportLocalsConvention defaults to "asIs" but can be set back to "camelCaseOnly" for migration purposes. The documentation

Regarding the changed tests: the snapshots have changed because the raw string output has changed. I could add a case which detects if the class name is a valid JavaScript identifier and simply outputs a plain export const className = "..." in that case but I would strongly recommend against it. This will cause knock on effects down the ecosystem because there will be 2 ways in which class names might be exported. We can also now export class names which are reserved JavaScript words, so this detection logic would end up being complex.

Users who are using CommonJS exports, or are not using the namedExports feature could still use old versions of nodejs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Please avoid breaking changes, changelog and etc, just add the new value for the exportLocalsConvention, i.e. asIsUsingAs (maybe better name, feel free here) and add docs about it and test (skip them when Node.js doesn't support it)

We don't want to make the major release right now. In the next major release we will consider the new value.

var _2e = \`xajoqP1d3SwrjJ4WEM8g\`;
export { _2e as \\"url\\" };
var _2f = \`Ix5nEHiVOsWuWxdx0twz \${___CSS_LOADER_ICSS_IMPORT_7____NAMED___[\\"scssClass\\"]}\`;
export { _2f as \\"main\\" };
Copy link
Member

Choose a reason for hiding this comment

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

Also please try to merge export into the one object to improve the parser perfomance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran some quick benchmarks and didn't see a noticeable impact between the two approaches. A project with 100,000 CSS class names will see an improvement of just 1ms by combining the export declarations. If you think that it's worth the increased complexity in css-loader, and the knock-on effects from increased complexity in plugins for this kind of performance gain then I will make the change though.

JavaScript, since 2021, can export any unicode name from modules. We can
take advantage of this to remove the restrictions on
`exportLocalsConvention` under `namedExports` mode.
@laverdet
Copy link
Contributor Author

Rebased and caught up with the main branch. Notes:

  • A new option for exportLocalsConvention is not needed. { namedExports: true, exportLocalsConvention: "asIs" } was never valid, the previous restriction is simply lifted
  • { exportLocalsConvention: "camelCaseOnly" } was added to the options for several unit tests in anticipation of the defaults changing in the future. This doesn't have any effect on the test results.

@@ -1566,6 +1566,7 @@ describe('"modules" option', () => {
it('should work with the "namedExport" option', async () => {
const compiler = getCompiler("./modules/namedExport/base/index.js", {
modules: {
exportLocalsConvention: "camelCaseOnly",
Copy link
Member

Choose a reason for hiding this comment

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

Why it was added? Sounds like a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned this in the comment above. The changes didn't have any effect on the unit test. I will remove.

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f9192ee) 96.04% compared to head (bbf0e7b) 96.15%.

Files Patch % Lines
src/utils.js 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1549      +/-   ##
==========================================
+ Coverage   96.04%   96.15%   +0.10%     
==========================================
  Files          10       10              
  Lines        1188     1195       +7     
  Branches      459      461       +2     
==========================================
+ Hits         1141     1149       +8     
+ Misses         38       37       -1     
  Partials        9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexander-akait
Copy link
Member

Let's finish webpack-contrib/mini-css-extract-plugin#1057, I want to make two releases together

Thank you

@alexander-akait alexander-akait merged commit 6f43929 into webpack-contrib:master Jan 23, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants