Re-enable CSS-in-JS tests #6556
Conversation
|
|
I guess, this PR is no-go until stylelint has dropped Node.js 12 and 14, since I also just discovered that similar problem in |
|
@hudochenkov Thanks for creating the pull request. I recommend using the v15 branch as a base because the branch has dropped Node.js 12 support. One question. Does the |
hudochenkov
force-pushed
the
postcss-styled-syntax
branch
from
1df3473
to
0f98378
Compare
OMG. When did it happen?! 🤦 On a quick inspection they serve exactly the same purpose — to parse styled-components. But feature-wise From a tech perspective they also have different approach:
I think |
|
they do indeed do the same. i published postcss-lit way back when these syntax changes happened and things were deprecated, and around the same time started on a styled-components iteration of that. more recently, i extracted most of the core of that to 43081j/postcss-js-core which can be used to build other css-in-js syntaxes. as part of that i spun up a styled-components/emotion package built on top of it. to clear something up too, fwiw its not a "hack", the expression interpolation you're talking about. initially, we had an iteration of the core which had its own tokeniser and soon realised we were basically going to be playing catchup forever - keeping the custom tokeniser in sync with the better maintained, better tested postcss tokenizer. on top of that we had quite a few test cases failing which were more difficult to solve and maintain through a custom tokenizer. ultimately though, nobody is tied to implementation nor should they be. if it was shown that a custom tokenizer could solve the interpolation problem in a better way than falling back to the postcss one, im sure thats the way we would go. i understand you've spent a bunch of time on this too. it has happened to all of us before, writing something in parallel to someone else writing roughly the same thing for the same problem. the packages i've published (js-core, lit, styled-components) aren't going anywhere since we already rely on them heavily and so do many other people. but im not against there being two options to people. or even better, if you could use what you have here and what you've learnt to contribute to postcss-js-core - so you can help out all syntaxes rather than just the styled-components one. nobody wants to part with the package they've built, but you're probably one of the only people around right now who has tackled this other than me, so it would actually be super nice to collaborate. |
Fun fact, I added
Sorry, I didn't mean to offend. I put it quotes, because I can't judge if that solution really a hack or not. CSS-in-JS is a wild west of CSS syntaxes, because it could have a lot of crazy stuff.
I found the opposite true. Initial approach with comments that you and me came up with didn't work out, so instead of playing a guess game I decided to rely on PostCSS tokenizer and parser. I marked changed bits (trick I found in SCSS tokenizer) so if there updates to default parser, it would be easily carried over without the fear of breaking added logic.
That's the tricky part. Because approach is very different it's difficult to contribute without re-doing everything again. I don't see much sense for me personally spend all this time again. Also, I don't feel comfortable with approach taken in I tried In comparison: let Component = styled.div`color: red; ${hello}`;
let Component = styled.div`${hello} color: red;`;
Maybe my solution with putting interpolations into
|
# Conflicts: # lib/utils/isStandardSyntaxDeclaration.js
You shouldn't mistake it for a competition, i was just pointing out that i started work on it a while ago, completely unrelated to anyone else having done so.
these and a few other things you said are all about implementation detail, which im really not tied to. its possible (and likely) i will revisit using a custom tokeniser at some point to see if there's a middle ground or a third solution that has best of both worlds. it really doesn't matter much to me how it is implemented as long as it is clean and does the job as expected. if you don't feel comfortable contributing to another repo or you feel like your work will be a sunk cost, thats fine, there's absolutely nothing wrong with having multiple solutions available in the world. obviously, i'd prefer that collaboration happens since nobody knows everything alone, but there's no bad feelings from it not happening.
stylelint in fact already had logic for accounting for root-level declarations and such, thanks to the old css-in-js implementations. i fixed some of this very recently to account for new syntaxes (since it was previously hard coded to only support the old syntaxes).
by choice. the strategy used for dealing with interpolation is documented as not supporting these kinds of unusual (syntactically) positions. that'd likely change if we ever changed the strategy but isn't any loss right now thanks to it being an uncommon enough use case. i feel like this has gone down a bit of a rabbit hole. you've spent most of your reply trying to compare the two implementations but i don't think that is necessary. if someone came along tomorrow and offered a better implementation into js-core, i would happily work with them to accept it. im well aware of limitations in the approach i took and chose to accept them for now due to them being mostly edge cases. at some point, someone (not necessarily me) will likely update it with a much better approach which will solve those edge cases. it is an ongoing project. i once converted parse5 to typescript (months of work) as another person did unknowingly in parallel. in the end, i threw mine away (but used it as reference) and we collaborate to the point where we both now maintain parse5. its not an easy conflict to solve. unfortunately in this case, all 3 of my packages are already used heavily in quite a few projects so its unlikely ill throw them away this time. but i still stand by that i'd love to collaborate and, if a tokeniser is a better way to go, get that into js-core (from scratch, not from your code or mine, together). |
|
@hudochenkov @43081j Thank you so much for all the work you have done for CSS-in-JS. I have no objection that we have different packages for the same purpose (although it might be better if they worked together). @hudochenkov I'm not sure why CI doesn't work, but I locally confirm that tests on Node.js 14 fail.
let lastComponent = foundNodes.filter((node) => isComponent(node, foundNodes)).at(-1); |
|
@ybiquitous purely from the point of view of the stylelint repo then, this PR would need reworking. the tests previously tested against css-in-js, a catch-all syntax for all JS based syntaxes. you can't replace it in all of those tests (and the source actually) with a new syntax which is not catch-all and actually only supports a subset. for example, stating that the tests would need to be updated, just like the file extension inference stuff such that there are multiple syntaxes rather than one monolith (in line with the whole point of moving away from css-in-js). otherwise you're essentially re-enabling the tests for a single syntax, ignoring the many others which exist. |
@ybiquitous
Currently all maintained CSS-in-JS PostCSS syntaxes are for template literals. We don't have syntaxes for object notation, or for JSX I think it still make sense to tests for existing syntaxes.
I think we need to create a new discussion issue for this. As now single file extension could mean a lot of different syntaxes. I reverted the change. |
…ss-in-js" This reverts commit 219dac5.
you could go ahead and add all the other existing syntaxes in that case.
agreed. if these tests are re-enabled, the following should be true IMO:
if it was updated to that state, a quick glance at the actual tests themselves also raises the question of if they should exist at all. IMHO each individual syntax should be responsible for its own tests from now on. it doesn't make much sense to put a burden on the stylelint maintainers to test external modules (and an increasing amount of them as time goes on). it isn't as simple as before when there was one module to test. i'd vote for throwing the whole lot away and leaving it to syntax maintainers to test their own code. |
If there is a real need for a specific syntax to be tested, then, of course, we can look into it. This PR goes from no tests for any CSS-in-JS syntax, to at least some tests.
I don't understand what does it mean. Could you elaborate, please?
I would disagree. I understand this statement as PostCSS should have test to all PostCSS plugins including Stylelint. Stylelint shouldn't have tests for any any non-default PostCSS parser. Similar for Babel, it should have tests for all plugins. Maybe I misunderstood. PostCSS syntaxes can't have tests for all possible tools that might use them. Stylelint is a bunch of PostCSS plugins (each rule is a PostCSS plugin). It takes PostCSS AST from whatever parser is used (default or custom) and make analysis of AST. If for some syntaxes it makes incorrect analysis, then Stylelint needs to be adjusted, if, of course, parse makes valid AST. So if logic is inside Stylelint repository, then tests also should be there. |
the tests you are updating were intended to test the css-in-js implementation (as in there was one at the time, and only one). this isn't true anymore and never will be. there are now multiple distinct syntaxes, not a catch-all css-in-js syntax. that means most or all of the tests you're re-enabling are no longer accurate, in that they belong in a test suite specific to your particular syntax, not in a test suite intended for all syntax. because of that, they're basically unit tests and integration tests for your own code, but living in the stylelint repo. that means we should probably do either of the following:
if all stylelint maintainers are happy with owning those test suites, we should do the former (which isn't far off what you have here, just reorganised a bit). you have to remember stylelint now should be able to operate against an AST without any knowledge of what created it or how. basically a separation of concerns. that should really mean its own tests are against any valid postcss syntax tree, rather than hard coding knowledge of individual syntaxes. |
They are to test Stylelint, that it does work correctly with specific custom syntax. Same as we have for
All tests that have Even when there was one catch-all syntax for CSS-in-JS. Tests were only for one of many flavors of CSS-in-JS from that package. Not for every possible flavor. Nothing changes. It's just that one particular flavor could be used from a separate package.
These are tests to test Stylelint ability to operate on the AST. It's not the test of AST, but Stylelint's logic working with the AST.
Agree. And Stylelint tries to do that. But in reality it's not possible. You will find in Stylelint many examples where specific syntax AST is treated in a special way (SCSS, Less). PostCSS AST is not very strict specifically to allow many of CSS-flavors to be processed. There is general structure and direction, but not strict structure. |
this is because really there are 3 syntaxes: sass, less (actually more or less the same as css), css. the css-in-js syntaxes are more like filetype syntaxes for purposes of extracting one of the above "source" syntaxes. whatever the css-in-js ones produce probably should be (and likely is) one of those 3 ASTs stylelint is aware of (all of which are really just postcss ASTs with more node types).
they are near enough integration tests for your syntax, testing that stylelint will work correctly with your syntax. that's why either the ones you have here should be in a test suite specific to your syntax, or should be in your repo rather than this one (that choice is upto stylelint maintainers as a group i suppose). what you have isn't far off that, it isn't a huge change.
mostly, if not entirely, sass/less. there shouldn't really be any css-in-js syntax specific logic in stylelint anymore (if there ever was). and one day, sass/less will likely also go away as they fall out of use/popularity. there's also a question now of whether stylelint should even be testing such things anymore. if a css-in-js syntax is thoroughly tested to produce a valid AST for stylelint, it probably doesn't make sense anymore to have individual per-rule tests (duplicates) per syntax, since every css-in-js syntax should produce the same AST (i.e. stylelint would only need one test). |
|
@hudochenkov @43081j Thanks a lot for providing great opinions and consideration. I think we should not rely on specific non-standard syntaxes as much as possible, even not only for CSS-in-JS syntaxes but also for SCSS or Less. The more external modules we depend on, the more difficult we maintain the codebase. It's important for the Stylelint core to focus only on standard CSS syntaxes for healthy maintenance, balancing with usability. I believe that it's unnecessary that each rule tests use non-standard syntaxes and it's necessary to remove them gradually. But some of So, I suggest removing currently skipped tests for a CSS-in-JS syntax in rule tests, and using (or installing) a syntax as needed for What do you think? |
|
i would agree. its more difficult for people like @hudochenkov and i (syntax maintainers) but if we don't do it this way, it means stylelint will continuously grow its test suite around codebases/behaviours it doesn't maintain. the tests we have for those css-in-js specific behaviours (as in #6565 for example) would still exist as part of stylelint, not to test the custom syntax in question but to test the generic support stylelint has in such a function (pretty much what you said about the although in those cases its still a little awkward as the easiest way to test such a codepath is by pulling in a specific syntax that hits it. it'd be nice if there was a way to test those branches "generically" so we know all syntaxes which conform to the same tree will work with it. not sure how we'd do that though it'd actually be nice to reduce the amount of syntax-specific support in stylelint as time goes on, too. in the hope that newer syntaxes we've yet to see will conform to standard CSS AST. |
|
I think we should document such decision in docs, so there is a place to look up decision from now onward. |
Agreed. I've opened PR #6572 to do that. |
None.
I've developed new PostCSS syntax to parse template literals CSS-in-JS like styled-components: postcss-styled-syntax.
I thought we could re-enable CSS-in-JS tests. I believe they were disabled because
@stylelint/postcss-css-in-jsis based on PostCSS 7, and because of lack of maintenance there were not much trust in it.I also propose to suggest
postcss-styled-syntaxin error message instead of deprecated@stylelint/postcss-css-in-js. Let me know if I need to revert it.