-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: container
query support via css-tree
extension
#8275
Conversation
@typhonrt is attempting to deploy a commit to the Svelte Team on Vercel. A member of the Team first needs to authorize it. |
container
query support via css-tree
extension
Thanks for your work on this @typhonrt! I'm currently testing your fork on a fairly complex component I've written using Sass (
Sass does support this already (I'm converting a component written in Astro to Svelte here, with Sass already in place), so I think this generally should be supported here. |
That |
It is. I have it running through the Sass preprocessor. Sass on its own supports this syntax., and does correctly compile it to a number. The error I'm getting in there terminal points to my pre-processed CSS, leading me to assume the parser hits my styling before Sass does. It also fails when using EDIT: Interestingly enough, the parse error in my browser and in my terminal are different; in my browser it points to the Sass interpolation, but in my terminal it specifically points to |
I also use Sass in my dev efforts, but have Test case: $swap: 50px;
@container (min-width: #{$swap}) {
main {
background: blue;
}
} Generates: @container (min-width: 50px) {
main.svelte-1c1au4r.svelte-1c1au4r {
background:blue
}
} |
Yah, sorry for the churn here, I think I was reporting the right error on the wrong place in my source code; getting conflicting items from my build tool. I think it's choking on |
Of interest in general is that Test case: $swap: 50px;
@media(min-width: calc(#{$swap} + 1)) {
main {
background: blue;
}
} Fails to parse:
However if you use calculation built into Sass it works: @media(min-width: #{$swap + 1}) {
main {
background: blue;
}
} Results in In general and quite likely why it will take a bit for |
So for my usecase, I need actual |
I have looked at other CSS parsers and I don't believe there is an OSS one that actually handles modern query syntax presently. For instance spin up AST Explorer You'll see that by default w/ current available versions of
By defining There also is an option in I don't think there is a clear cut OSS CSS AST parser to switch to... I wished it could be as easy to suggest a workaround by commenting out the The Definitely straying from the purpose of this PR though. It is good to realize that currently there are more cases of modern CSS syntax that trip up |
So, I did do a bunch of research / investigation yesterday. Functions that return a single number / value are valid where a single value can be used, but it's non-obvious from the media query / container query specs along with various conflicting historical discussion around the web. For the MQ spec I linked to the section where this is implied:
There is no mention of this in the CQ spec. It would be nice if the specs and MDN docs for MQ / CQ gave examples of this to make it more obvious, but they don't. See calc() expressions in the MQ browser compatibility table showing wide browser support. The single value CSS functions that apply are: I found discussion for I don't see a problem in adding support for those 4 functions in the parsing code of this PR and I will work on implementation aspects this weekend and release another update to my temporary Svelte fork on NPM so folks can test it out. I will do a bit more research to verify if functions are only applicable to some MQ / CQ properties and not others. Given communication and support from the Svelte maintainers I don't see a problem with submitting an additional PR that addresses media queries using the same |
Much needed feature, thank you very much! I am planning to use container queries on production app and was surprised with the error message. I do hope this will be merged soon! |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
So, I have added single value CSS function support to the parsing. The function expression (content inside parenthesis) is not parsed, but verification of @container test-container (calc(400px + 1px) <= width < calc(500px + 1px)) {} Waiting on maintainer review(s). This PR has been kept up to date w/ latest commits to Svelte as of this message. @Snugug and others you can test out this functionality now as I have a temporary fork of Svelte w/ this PR available with this NPM package. You can load it in a Svelte project with the following in "overrides": {
"svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq2"
}, |
If that's any help - I've tested it out on some basic container queries over the weekend on my project and everything seemed to work like it suppose to. |
@typhonrt I can confirm that with the new version, my weird |
Yet another minor update.. The PR is up to date w/ mainline Svelte as of this message. A new external version of Svelte "overrides": {
"svelte": "npm:@typhonjs-svelte/svelte@3.57.0-cq"
}, Still waiting on Svelte maintainers to take a look. I'm going to ping you, @baseballyama, as you added the |
Thank you for your effort for this! Can you please explain us why we need to send a PR to In my understanding is that the best way is to support |
If you read the The There has been no communication from the maintainer since then and the queries branch stalled out December 15th. It is unknown when The reason to extend This PR includes a test for the added CQ support: I have been releasing a fork of Svelte w/ the CQ PR applied for a while now and folks are starting to use it, so it has been tested externally with Svelte and SvelteKit projects. |
Thank you for your explanation! I have one more question. /** app.css */
@container (max-width: 600px) {
.container p {
color: red;
}
} <!-- App.svelte -->
<div class="container">
<p>Some text here.</p>
</div>
<style>
.container {
container-type: size;
}
</style>
|
It is you are shipping a component library / UI framework as I do. There are workarounds so to speak, but only reasonable for an end project to utilize and they are clunky at best. Being able to scope CQ CSS to components is very useful. This PR more or less is the "least worst" interim solution until the mainline release of I do understand the potential maintenance burden. I spent a fair amount of time to understand the internals of |
As a Svelte user, I agree with you. So I want to wait Already you released a forked version of Svelte. So people who need to support This is my personal opinion for now. |
I do appreciate you providing input as I'm trying to start a conversation and hopefully gain some sort of consensus and I do hope other Svelte maintainers can be a part of this discussion. I certainly will try to answer some of your thoughts diplomatically as possible. :)
It is 100% broken right now. Something that works and is well tested providing an interim solution is better than 100% broken. This PR has a full test case. That it could break is a hypothetical. If such a problem occurred I'd be more than happy to fix it.
No resources for the core team need to be expended as this PR is a complete solution other than discussing the merit of the approach and recognizing that container queries are important to support. I'd argue isn't the whole point of the quick v4 to v5 to modernize Svelte? Then supporting modern CSS should be included in that and there is no reason to wait. Container queries is a very important development and the pathway to creating really complex and dynamic components. There is also no burden in the planned switch to ESM / JSDoc. Swap the file extensions to
Do you have a date in mind? June this year? How long? Firefox shipped it last month; Firefox...
How many need to download it weekly? I should also note that the current mechanism for including my fork as I spent over a month last November developing a complex compound component (advanced color picker) that uses container queries. CQ is a game changer. I can't release that component in my library; it is commented out of the distribution. Folks can uncomment that under the It would be great to get other input and I do hope folks chime in within the window to get things into Svelte v3 then onto v4 / v5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently leaning towards merging the PR because as you say it's the least worse option. Waiting: unclear how long we have to wait. Switching parsers: Probably even more work, and may have unforseen drawbacks.
I gave the PR a quick review and have a few questions
- Can we be sure that these
this
methods you're using are not internal (I'm ok with semi-public) and can change at any moment? (CI should save us from accidentally pushin a broken version, but still) - Can you give maintainers write access to the branch? I wanted to do some quick formatting/snake_case code changes but can't
- (also one more in a comment, see below)
@@ -0,0 +1,43 @@ | |||
// @ts-nocheck | |||
// Note: Must import from the `css-tree` browser bundled distribution due to `createRequire` usage if importing from | |||
// `css-tree` Node module directly. This allows the production build of Svelte to work correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on that createRequire
usage thing? I'm not sure what this is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just repeating info I included in the post below:
So, the Node version of css-tree
when including fork
has a path that invokes createRequire
in data.js
:
https://github.com/csstree/csstree/blob/master/lib/data.js#L4-L7
The ESM bundled / browser version of css-tree
has all of those resources in the ESM bundle thus doesn't depend on Node specific functionality to import a local JSON file.
When building Svelte for production (not local testing) it causes an issue to use the Node version of css-tree
when accessing fork
for extension as those JSON files can not be referenced by the production / zero-dependency build of Svelte as the bundling / distribution process doesn't include the internal css-tree
JSON files referenced. When not producing a production build css-tree
and a couple of other Node dependencies are not bundled / marked external thus it works.
https://github.com/sveltejs/svelte/blob/master/rollup.config.js#L144-L146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Rollup this sounds ok. Just to double check: Once we go unbundled ESM then can't do this anymore like this. css-tree
will be a regular dependency which means we have to use the public imports. But that would be ok because the JSON file will be available then within css-tree
(because it's installed as a regular dependency), correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably when going unbundled ESM the dependencies previously bundled that are marked external in the local test build will be moved to package.json
dependencies
; likely: acorn
, css-tree
, and magic-string
. In this case it will certainly be possible to directly reference the Node import path IE import { fork } from 'css-tree';
Outside of course SvelteKit taking the unbundled ESM approach, some day I'd be glad to learn more about the impetus for native ESM vs TS. I too have always been a native ESM proponent and all my Svelte UI framework / library work is native ESM. I also work on tooling to be able to generate bundled TS declarations from ESM source + JSDoc. Not saying that is applicable in Svelte's future, but a curious conversation overall for another day.
There is no clear front runner for a better OSS parser. I have done an informal survey and most are still catching up to the latest standards. It would be a lot more work to replace the parser for sure.
The You can see the This is the internal API exposed to the different AST node modules.
I'd be glad to make the changes later today. I did use the code style found in I also can look into giving access to maintainers, but this little cleanup task is something I can handle too.
So, the Node version of The ESM bundled / browser version of When building Svelte for production (not local testing) it causes an issue to use the Node version of https://github.com/sveltejs/svelte/blob/master/rollup.config.js#L144-L146 |
Re formating/snake_case - sure, you can also do it yourself 👍 If you're using VSCode, there's "convert indentation to tabs" which should help you - right now it's spaces mixed with tabs, and after conversion indentation is wrong in some places. Regarding snake_case, it's only a handful of variables/functions that would need a rename.
|
I'm pretty sure I got all the code formatting correct in last commit. Let me know if there is anything else I can improve to move things forward. I'm certainly OK w/ adding a healthy bit of JSDoc if that makes sense right now. |
Closes #6969. As discussed there, container query support is quite useful to add to Svelte as it is now broadly available with Firefox releasing support imminently w/ FF v110 this upcoming week (~Feb 14th). Chrome has had support since ~Aug '22. The central issue is that
css-tree
which is a dependency for CSS AST parsing is significantly lagging behind on adding more recent features such as container query support. Ample time has been given to the maintainer to updatecss-tree
and I do have every confidence that in timecss-tree
will receive a new major version with all sorts of modern CSS syntax supported including container queries. This PR provides an interim solution for what Svelte needs to support container queries now.This PR broadly helps the entire Svelte ecosystem including SvelteKit.
Changes
src/compiler/parse/read/style.ts
: Switch to locally forkedcss-tree
w/ CQ support.src/compiler/parse/read/css-tree-cq/css_tree_parse.ts
:fork
css-tree
w/ local extension.src/compiler/parse/read/css-tree-cq/node
:css-tree
new CQ nodes.src/compiler/compile/css/Stylesheet.ts
: Trivial one line change to enable CQ support in Svelte.test/css/samples/container-query
: Parsing test case.Impact
No downsides that I'm aware of regarding these changes. Simply that Svelte can now ship
3.56.0
w/ CQ support. I have thoroughly tested the code and added an additional test to thesvelte
repo. Because I'm using thefork
capability ofcss-tree
to extend it thecss-tree
version can keep being bumped if new versions come out without CQ support. However, when CQ support is added my local extensions can be removed and an easy swap back to the officialcss-tree
parse capability is trivial.Testing
A new test was added in the
css
sectioncontainer-query
that runs through several common usage permutations. Just like themedia-query
test it contains positive tests that ensure parsing.css-tree
ExtensionThe bulk of this PR is local extension of
css-tree
using thefork
capability ofcss-tree
to add new parsing nodes that implement the parsing of@container
. The benefit of taking this approach is that the temporary modifications necessary to extendcss-tree
are committed to the Svelte repo and a separate hard fork ofcss-tree
isn't necessary. Thecss-tree
version can continue to be bumped as necessary and whencss-tree
supports container queries the local extension can be removed.It is unknown when
css-tree
will be updated to support container queries and other more recent style options like media range query syntax. However, whencss-tree
does offer support a one line change and removal of the extensions provided in this PR is trivial.I would be glad to discuss the
css-tree
specific parsing code added. I certainly recognize that an internal knowledge ofcss-tree
is probably not widely held by most of the Svelte maintainers. My extension follows current patterns used incss-tree
itself.Instead of duplicating all conversation from #6969 Please refer to that issue for pertinent prior discussion. As indicated in the last few comments there (#6969 (comment)) I have released a temporary version of Svelte 3.55.1 on NPM that can be used to test these changes now in live environments. Two other community members responded that these changes work as intended including a SvelteKit trial.
Tagging: @tanhauhau (you created the
css-tree
issue in response to the initial triage of #6969) and @dummdidumm. Please include any other relevant maintainers for review.