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

rewrite (wip) #532

Closed
wants to merge 49 commits into from
Closed

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Jul 19, 2023

@RyanZim this set of changes is roughly where I want to end up.
It combines #531 and #530 and fixes a lot of previously unknown bugs.

It is validated by comparing the behavior against Firefox https://github.com/romainmenke/css-import-tests

Summary:

  • we no longer combine @media rules into a single rule (this was broken)
  • we no longer combine @layer rules into a single rule (this was broken)
  • added support for @supports
  • added support for cyclical imports when skipDuplicates is false
  • added support for data urls with url encoded contents (previously only base64)
  • fixed data urls that contain @imports with relative urls (this must not work, and previously did, this should affect no one)

If this change does go through it would make postcss-import vastly superior to lightningcss, esbuild, ... which would be a nice incentive for those packages to fix their issues.

This is a very large rewrite however and I am unsure if it makes sense to ship this here.
I can not foresee if people depended in weird ways on behavior that, to me, is a bug.

An alternative I am considering is to strip down some of the features and create a new plugin that is more focussed on matching @import as it works in browsers.

thoughts?

@RyanZim
Copy link
Collaborator

RyanZim commented Jul 19, 2023

My apologies for my slowness here; I've been quite busy, and haven't had time to review your PRs as promptly as I'd wish.

I can not foresee if people depended in weird ways on behavior that, to me, is a bug.

We'll just ship a major release, and people can deal with any incompatibilities.

An alternative I am considering is to strip down some of the features and create a new plugin that is more focussed on matching @import as it works in browsers.

Can you elaborate on this further?

@romainmenke
Copy link
Collaborator Author

An alternative I am considering is to strip down some of the features and create a new plugin that is more focussed on matching @import as it works in browsers.

Can you elaborate on this further?

This package also supports a few non-standard features.

  • first import over all imports with skipDuplicates: true
  • imports of npm packages

I personally tend to favor standard/specification defined behavior when creating and updating packages like these.

But I want to be respectful of all current users.
I don't want to steer this package into an unexpected direction.

Having two packages, each with a different focus can be a good alternative.
I don't mind creating a second package if this is best overal for users.

As far as I can tell these changes aren't breaking except for extreme edge cases, but I try to be mindful of my blindspots.

@romainmenke romainmenke closed this Dec 9, 2023
@romainmenke romainmenke deleted the wip-rewrite--courteous-antelope-d8443985a8 branch December 9, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants