Fix problem #960

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

Jump to bottom

Closed

zyrong wants to merge 3 commits into developit:master from zyrong:master

Closed

Fix problem #960

zyrong wants to merge 3 commits into developit:master from zyrong:master

Conversation

@zyrong

Copy link

Contributor

@zyrong zyrong commented May 16, 2022

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.

zyrong added 3 commits

May 16, 2022 00:26

@zyrong


 fix: css compilation problems when multiple entries

4580e5f

@zyrong


 test: update test snapshots

6c17bb4

@zyrong


 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.

@changeset-bot

Copy link

changeset-bot bot commented May 16, 2022

🦋 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

Copy link

Collaborator

rschristian commented May 16, 2022 •

edited

Loading

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

Copy link

Contributor Author

zyrong commented May 16, 2022

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

Copy link

Collaborator

rschristian commented May 16, 2022

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

rschristian reviewed

May 16, 2022

View reviewed changes

test/__snapshots__/index.test.js.snap

Comment on lines -435 to +442

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

Copy link

Collaborator

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

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

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?

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

Comment on lines -5 to +6

"regex": "^_"

"regex": "^_",

"reserved": []

Copy link

Collaborator

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

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

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

src/index.js

const shebang = {};

function createConfig(options, entry, format, writeMeta) {

const entryFileName = basename(entry, extname(entry));

Copy link

Collaborator

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

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

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.

src/index.js

Comment on lines -505 to +478

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

resolve(outputDir,`${entryFileName}.css`),

Copy link

Collaborator

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

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

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.

Copy link

Contributor Author

@zyrong zyrong May 17, 2022

Choose a reason for hiding this comment

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

How about changing it to this

@zyrong zyrong mentioned this pull request

May 17, 2022

fix: css compilation problems when multiple entries #961

Merged

@zyrong

Copy link

Contributor Author

zyrong commented Jun 3, 2022

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

Copy link

Collaborator

rschristian commented Jun 3, 2022

Superseded by #961 & #966

@rschristian rschristian closed this

Jun 3, 2022

Labels

None yet

2 participants

@zyrong @rschristian

Fix problem #960

Fix problem #960

Uh oh!

Conversation

@zyrong zyrong commented May 16, 2022

Uh oh!

changeset-bot bot commented May 16, 2022

🦋 Changeset detected

Uh oh!

rschristian commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyrong commented May 16, 2022

Uh oh!

rschristian commented May 16, 2022

Uh oh!

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

Uh oh!

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

Uh oh!

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

Uh oh!

@rschristian rschristian May 16, 2022

Choose a reason for hiding this comment

Uh oh!

@zyrong zyrong May 17, 2022

Choose a reason for hiding this comment

Uh oh!

zyrong commented Jun 3, 2022

Uh oh!

rschristian commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rschristian commented May 16, 2022 •

edited

Loading