Fix problem #960

zyrong · 2022-05-16T05:16:36Z

fix: css compilation problems when multiple entries

fix: Fix problems related to use rollup-plugin-terser

The nameCache obtained in writebundle hook is not a reference to terseroptions
JSON.stringify(nameCache) requires converting regex to a string first.

1. The nameCache obtained in writebundle hook is not a reference to terseroptions 2. JSON.stringify(nameCache) requires converting regex to a string first.

changeset-bot · 2022-05-16T05:16:38Z

🦋 Changeset detected

Latest commit: b0f2291

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
microbundle	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

rschristian · 2022-05-16T05:23:23Z

Would you mind splitting half of this out into another PR? Fixing two (seemingly) different issues in one PR makes it a bit hard to review and follow.

Edit: Also helps the history. With both fixes in one PR, it's a bit murky which bit of code fixes what issue. If there's ever problems, it helps if it's clear to what issue a line was altered/added.

For both, do you mind writing up issues showcasing the problems you're experiencing?

css compilation problems when multiple entries

This is a bit vague, for example. It's difficult to review the work because I don't know what issues you've run into or if your added test cases adequately check your fix.

Thanks

zyrong · 2022-05-16T11:53:12Z

Would you mind splitting half of this out into another PR? Fixing two (seemingly) different issues in one PR makes it a bit hard to review and follow.

Edit: Also helps the history. With both fixes in one PR, it's a bit murky which bit of code fixes what issue. If there's ever problems, it helps if it's clear to what issue a line was altered/added.

For both, do you mind writing up issues showcasing the problems you're experiencing?

css compilation problems when multiple entries

This is a bit vague, for example. It's difficult to review the work because I don't know what issues you've run into or if your added test cases adequately check your fix.

Thanks

What should I do to split into two pr

This commit fixes that when different entries reference different CSS, only the related CSS of the first entry will be packaged. For specific changes, please see this commit. I also added the corresponding test case
fix: css compilation problems when multiple entries
This commit fixes the problems related to terser namecache. When terser compresses the code, the damaged variable names will be written to the namecache. Finally, the namecache will be written to the file system in this code, but after my test, it doesn't seem to work here
fix: Fix problems related to use rollup-plugin-terser

rschristian · 2022-05-16T18:05:02Z

What should I do to split into two pr

CSS fixes in one, Terser thing in another. Specific commits get lost as PRs are squashed and merged.

rschristian · 2022-05-16T20:54:07Z

test/__snapshots__/index.test.js.snap

-    basic-css.css
-    basic-css.css.map
    basic-css.esm.mjs
    basic-css.esm.mjs.map
    basic-css.js
    basic-css.js.map
    basic-css.umd.js
    basic-css.umd.js.map
+    index.css
+    index.css.map


This shouldn't be a patch if file names are changing without any user behavior alterations.

In spirit of keeping naming consistent, CSS output should also match every other file output by default, not fallback to index IMO.

Can we revert this bit, or switch this to a minor please?

rschristian · 2022-05-16T21:00:48Z

test/fixtures/mangle-json-file/mangle.json

-        "regex": "^_"
+        "regex": "^_",
+        "reserved": []


Did these mangle files need to be updated in order to ensure tests still pass? If so, this too is a breaking change and the changeset should be corrected to mark this as a minor (Semver 0-major, breaking change = minor bump).

rschristian · 2022-05-16T21:13:56Z

src/index.js

@@ -333,6 +328,7 @@ function getMain({ options, entry, format }) {
 const shebang = {};

 function createConfig(options, entry, format, writeMeta) {
+	const entryFileName = basename(entry, extname(entry));


This is incorrect. Please use the existing naming setup that we had, it ensures names are properly stripped:

microbundle/src/index.js

Line 505 in 9a4e2b2

options.output.replace(EXTENSION, '.css'),

index.esm.js as an input needs to become index; yours would have it become index.esm, so index.esm.css would be output. We need to strip indicators towards ES module format, i.e., cjs, es[m], etc.

rschristian · 2022-05-16T21:22:43Z

src/index.js

-							options.output.replace(EXTENSION, '.css'),
+							resolve(outputDir, `${entryFileName}.css`),


I'd rather we not use outputDir as it isn't guaranteed to match up with expectations in the future (see #896)

CSS should continue to be output according to the result of getOutput for resilience IMO.

How about changing it to this

zyrong · 2022-06-03T14:45:53Z

What should I do to split into two pr

CSS fixes in one, Terser thing in another. Specific commits get lost as PRs are squashed and merged.

I have split it into two independent PRs and modified them. Can you make some suggestions or reply to the changes

rschristian · 2022-06-03T22:43:16Z

Superseded by #961 & #966

zyrong added 3 commits May 16, 2022 00:26

fix: css compilation problems when multiple entries

4580e5f

test: update test snapshots

6c17bb4

fix: Fix problems related to use rollup-plugin-terser

b0f2291

1. The nameCache obtained in writebundle hook is not a reference to terseroptions 2. JSON.stringify(nameCache) requires converting regex to a string first.

rschristian reviewed May 16, 2022

View reviewed changes

zyrong mentioned this pull request May 17, 2022

fix: css compilation problems when multiple entries #961

Merged

rschristian closed this Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problem #960

Fix problem #960

zyrong commented May 16, 2022

changeset-bot bot commented May 16, 2022

rschristian commented May 16, 2022 •

edited

Loading

zyrong commented May 16, 2022

rschristian commented May 16, 2022

rschristian May 16, 2022

rschristian May 16, 2022

rschristian May 16, 2022

rschristian May 16, 2022

zyrong May 17, 2022

zyrong commented Jun 3, 2022

rschristian commented Jun 3, 2022

		options.output.replace(EXTENSION, '.css'),
		resolve(outputDir, `${entryFileName}.css`),

Fix problem #960

Fix problem #960

Conversation

zyrong commented May 16, 2022

changeset-bot bot commented May 16, 2022

🦋 Changeset detected

rschristian commented May 16, 2022 • edited Loading

zyrong commented May 16, 2022

rschristian commented May 16, 2022

rschristian May 16, 2022

Choose a reason for hiding this comment

rschristian May 16, 2022

Choose a reason for hiding this comment

rschristian May 16, 2022

Choose a reason for hiding this comment

rschristian May 16, 2022

Choose a reason for hiding this comment

zyrong May 17, 2022

Choose a reason for hiding this comment

zyrong commented Jun 3, 2022

rschristian commented Jun 3, 2022

rschristian commented May 16, 2022 •

edited

Loading