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

Internalizes parser function from css dependencie #443

Closed
wants to merge 2 commits into from

Conversation

mechamobau
Copy link

@mechamobau mechamobau commented Mar 4, 2022

What:

Internalizes CSS parser function from css dependencie.

Why:

Because the library has been not maintained anymore. And this lib uses another dependency called source-maps-resolver (that is not consumed by @testing-library/jest-dom) that is officially deprecated too, showing an annoying warning when jest-dom is used.

image

How:

Extracting the parse function from css

@gnapse
Copy link
Member

gnapse commented Mar 7, 2022

Hmmm, I'm hesitant to just bring in that entire library's core into our code as a solution. A few reasons come to mind:

  • First thing I'd do is see if there are other libraries that could help us without us having to bring in this code to be maintained by ourselves. Accepting the burden to maintain this is too much.
  • If we really need to fork/copy over that library's code into ours, some more considerations come to mind:
    • Lack of tests (which could be solved by bringing in the tests from that lib too)
    • Too broad scope: that code has support for parsing the entire CSS language. We only really need to parse the rules inside a selector. I'd rather trim their code down to the parts that take care of this, or get inspired by how they do it and do that ourselves. And then use/adapt their very tests on this to bring them to our library.

I know these are all more difficult in the long run than merely copying this over blindly right now. But they'll pay off in the long run.

@MichaelDeBoey
Copy link
Member

@gnapse What do you think would be the best approach on pushing this forward?

@Asphiii
Copy link

Asphiii commented May 12, 2022

Any news on that?

@suutari
Copy link

suutari commented Jun 23, 2022

FYI there seems to be a new fork of the css lib in progress. See reworkcss/css#164
and https://github.com/adobe/css-tools. Maybe this @adobe/css-tools would be better maintained? Though currently it seems to be in Release Candidate phase still.

@holblin
Copy link
Contributor

holblin commented Jul 14, 2022

The version 4.0.0 has been released today :-)
https://www.npmjs.com/package/@adobe/css-tools

This doesn't contain the source-map support but otherwise, it is a 1o1 replacement with up to 25% performance improvement.

Let me know if there is any error using this new version :-)

@holblin
Copy link
Contributor

holblin commented Jul 15, 2022

I propose that PR to fix the problem: #470

@MichaelDeBoey
Copy link
Member

Duplicate of #470

@MichaelDeBoey MichaelDeBoey marked this as a duplicate of #470 Aug 5, 2022
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.

6 participants