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] Compiler option css: 'none' support #7890

Closed
wants to merge 0 commits into from

Conversation

maxiruani
Copy link
Contributor

@maxiruani maxiruani commented Sep 24, 2022

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

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

site/content/docs/04-compile-time.md Outdated Show resolved Hide resolved
@maxiruani
Copy link
Contributor Author

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

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

@@ -16,6 +16,12 @@ export default function read_style(parser: Parser, start: number, attributes: No

const content_end = parser.index;

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

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

@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

@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

@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

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

3 participants