Skip to content

[feat] Compiler option css: 'none' support#7890

Closed
maxiruani wants to merge 0 commit intosveltejs:masterfrom
maxiruani:master
Closed

[feat] Compiler option css: 'none' support#7890
maxiruani wants to merge 0 commit intosveltejs:masterfrom
maxiruani:master

Conversation

@maxiruani
Copy link
Contributor

@maxiruani maxiruani commented Sep 24, 2022 •
edited
Loading

Why?

I found that generating client side hydratable code with css: false or css: true end up processing styles entirely unnecessary.
When hydrating SSR components, the styles are commonly already generated and there is no need to generate it again for the client side bundle.
This will speed up the build development process because it completely avoids processing it.

Details

  • I have also added support for more descriptive 'internal' (formerly true) and 'external' (formerly false) css option values. It still supports boolean values but it raises deprecated warning.
  • Added support for 'none' css option.
  • Added test for css: 'none' option.

References

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

benmccann
benmccann reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I like this change. The true/false values have always been confusing to me and the performance enhancement will be nice as well

@maxiruani maxiruani requested a review from benmccann September 26, 2022 19:29
@maxiruani
Copy link
Contributor Author

maxiruani commented Sep 26, 2022

@benmccann I have changed the code based on your comments. Don't know why some CI task failed, it seems to be something "random". The previous CI test passed and nothing significant changed.

benmccann
benmccann approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

don't worry about the test failures. they're probably just flaky tests. I'll rerun the failing jobs and expect they'll pass

@Prinzhorn Prinzhorn mentioned this pull request Sep 29, 2022
Closed
Prinzhorn
Prinzhorn reviewed Sep 29, 2022
View reviewed changes
src/compiler/parse/read/style.ts Outdated

// discard styles when css is disabled
if (parser.css_mode === 'none') {
parser.read(/<\/style\s*>/);
Copy link
Contributor

@Prinzhorn Prinzhorn Sep 29, 2022

Choose a reason for hiding this comment

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

Maybe put the regex in a const so that the three (line 11 and 76) will never diverge?

@benmccann
Copy link
Member

benmccann commented Oct 5, 2022

@maxiruani it looks like there's a merge conflict in the docs now. Can you update against the latest master?

@maxiruani
Copy link
Contributor Author

maxiruani commented Oct 6, 2022

@benmccann I made a rebase from master. Don't know why the diff tab shows changes that are already on master. Do you know how to avoid that?

@benmccann
Copy link
Member

benmccann commented Oct 6, 2022

@maxiruani it looks like there's a couple things that may have gone wrong, but probably the biggest mistake was making the PR on the master branch. Normally you should create a new branch for each PR

I would first recommend creating a new branch as a backup so that you have a copy of the code should anything else go wrong:

git checkout -b css-none-backup

And then create a new branch to work on:

git checkout -b css-none

At this point there's a few things you can try. You could try rewinding the branch to remove the latest commits:

git reset -hard c5e8f3ce803aee97a368f609f6dd3099dc901457

You're going to have to close this PR and open a new one on the new branch.

I'd also recommend blasting away your master branch and getting a fresh copy:

git branch -D master
git remote add upstream git@github.com:sveltejs/svelte.git
git fetch upstream

I'm on Discord under the same username. Feel free to ask in #contributing and hopefully we can help get you sorted

@maxiruani maxiruani closed this Oct 6, 2022
@maxiruani maxiruani mentioned this pull request Oct 6, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benmccann benmccann benmccann approved these changes

+1 more reviewer

@Prinzhorn Prinzhorn Prinzhorn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maxiruani @benmccann @Prinzhorn