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

fix: ignore single css chunk (close #688) #689

Merged
merged 1 commit into from
Jan 24, 2021

Conversation

jas0ncn
Copy link
Contributor

@jas0ncn jas0ncn commented Jan 13, 2021

Summary

Ignore single css chunk when RequiredChunks added into HTML output during SSR.

Issue: #688

@theKashey
Copy link
Collaborator

There is just single one issue with test preventing merging this PR to a master branch 😭

@@ -171,6 +171,11 @@ function isValidChunkAsset(chunkAsset) {
return chunkAsset.scriptType && !HOT_UPDATE_REGEXP.test(chunkAsset.filename)
}

const CSS_FILE = /\.css$/
Copy link

@cahnory cahnory Jan 15, 2021

Choose a reason for hiding this comment

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

Rather than testing if all files are CSS, shouldn't it be tested if all files are not JavaScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I will modify Regexp and test js.

@jas0ncn
Copy link
Contributor Author

jas0ncn commented Jan 15, 2021

There is just single one issue with test preventing merging this PR to a master branch 😭

Should I generate new __fixtures__ folder to test this case, or just modify stats.json directly? This case needs webpack5 and I think maybe I shouldn't update webpack to v5 of the server-side-rendering example.

@cahnory
Copy link

cahnory commented Jan 19, 2021

There is just single one issue with test preventing merging this PR to a master branch 😭

Should I generate new __fixtures__ folder to test this case, or just modify stats.json directly? This case needs webpack5 and I think maybe I shouldn't update webpack to v5 of the server-side-rendering example.

Not sure if it's the right answer but in my (now closed 😢) PR I chose to modify the fixtures:
https://github.com/gregberge/loadable-components/pull/655/files#diff-7979dacf4945ebda3d5b355e934617b7bf14dbe6ec1996030bce2d413503349a

I think if your code is working but not passing the tests, it's better to edit tests if possible (tests adapt to code, not the other way).

@theKashey
Copy link
Collaborator

merging to continue development from my side...

@theKashey theKashey merged commit 6926e19 into gregberge:master Jan 24, 2021
@theKashey
Copy link
Collaborator

Should I generate new fixtures folder to test this case, or just modify stats.json directly?

fixtures should be always generated from some source code, but look like that moment got lost somewhere.
After regenerating status from the expected source(examples/server-side-rendering) I have 18 broken tests....

@theKashey
Copy link
Collaborator

While there is no test yet for this use case, it will be very good to double check that current version in master works and/or modify https://github.com/gregberge/loadable-components/tree/master/examples/server-side-rendering to produce the extended stats.
👉there is yet undocumented script scripts/prepare to do perform this operation.

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.

3 participants