postcss-import icon indicating copy to clipboard operation
postcss-import copied to clipboard

fix import with layer

Open romainmenke opened this issue 3 years ago • 4 comments

fixes : https://github.com/postcss/postcss-import/issues/495

@RyanZim it still feels a bit like the complexity (which I added) is exploding and the overal shape of the code base isn't optimal. But I also do not dislike this fix :)

Important spec bits : https://www.w3.org/TR/css-cascade-5/#at-import

the optional layer keyword or layer() function assigns the contents of the style sheet into its own anonymous cascade layer or into the named cascade layer.

The layer is added to the layer order even if the import fails to load the stylesheet, but is subject to any import conditions (just as if declared by an @layer rule wrapped in the appropriate conditional group rules).the optional layer keyword or layer() function assigns the contents of the style sheet into its own anonymous cascade layer or into the named cascade layer.

The layer is added to the layer order even if the import fails to load the stylesheet, but is subject to any import conditions (just as if declared by an @layer rule wrapped in the appropriate conditional group rules).

Which means that @import url('foo.css') layer(A) (min-width: 320px) becomes :

@media (min-width: 320px) {
  @layer A {
    /* imported styles */
  }
}

romainmenke avatar Jun 27 '22 17:06 romainmenke

@RyanZim do you have time one of the following days to review this? 🙇

romainmenke avatar Jul 25 '22 21:07 romainmenke

@romainmenke My apologies for my slowness here; I've been quite busy. Then, I was in a fairly serious accident last week, so that's put things even more on behind. I have not forgotten; I will try to review in the coming days.

RyanZim avatar Jul 25 '22 22:07 RyanZim

@RyanZim Sorry to hear that :/ Take all the time you need, this is just code and not important.

I hope you are ok 💐

romainmenke avatar Jul 25 '22 22:07 romainmenke

@RyanZim Thank you for the review and happy to hear things are getting better on your end 🎉

romainmenke avatar Aug 05 '22 08:08 romainmenke

I'm hesitant about the naming of anonymous layers, and what unexpected problems that may create. I'm not super familiar with @layer myself, so I don't know all the edge cases.

RyanZim avatar Aug 13 '22 20:08 RyanZim

I'm hesitant about the naming of anonymous layers, and what unexpected problems that may create. I'm not super familiar with @layer myself, so I don't know all the edge cases.

I actually am also unsure about this. Thank you for calling this out.

We could make anonymous layered imports unsupported for now. That way we avoid unforeseen edge cases while still shipping an improved version.

We can iterate on this later to add support for anonymous layers. This would only affect @import "foo.css" layer; not @layer {}.

The current version might be incorrect when an author has multiple stylesheets, all with anonymous layered imports. There is no way for this plugin to be aware of that and the counter approach doesn't produce truly unique layer names.

I did however want to avoid using true random values as these make builds non-reproducible.


From a technical standpoint an anonymous layer is equivalent to a named layer that only appears once.

So a sufficiently uniquely named layer is the same as an anonymous layer.

Making this sufficiently unique is the tricky part.

romainmenke avatar Aug 13 '22 20:08 romainmenke

OK, compromise proposal: we don't support anonymous layers by default. To enable support, you must pass an option which is a function, that is passed the index, and must return the layer name. That way, if you want our current index solution, you could pass:

(index) => `imported-anon-layer-${index}`

If you have multiple stylesheets, you could use different prefixes. Maybe we should also pass the root filename we're processing, for those who want to incorporate that into the name. Alternately, if someone doesn't care about deterministic builds, they could simply provide a random string generator (with or without a prefix).

I realize this is a bit complicated, but it also provides maximum flexibility; thoughts? Also, sane naming suggestions for such an option?

RyanZim avatar Aug 16 '22 00:08 RyanZim

I like this! I will try to implement this in the next few days :)

romainmenke avatar Aug 16 '22 05:08 romainmenke

@RyanZim I think this is ready for review (when you have time). For the moment I have settled on nameLayer for the plugin option.

Other candidates :

  • nameLayer
  • deAnonymizeLayer
  • generateLayerName

I wasn't sure how to pass the root file name considering that it might not be set. At the moment I am using the import file name as a second argument.

romainmenke avatar Aug 24 '22 15:08 romainmenke

@RyanZim Thank you for the feedback. I've made the necessary changes.

Hopefully all good now :)

romainmenke avatar Aug 29 '22 18:08 romainmenke

Merged; this is technically semver-major, so I'm going to try to bundle this with a Node version requirement bump to release.

RyanZim avatar Aug 30 '22 19:08 RyanZim

Thank you for this @RyanZim 🙇

romainmenke avatar Aug 30 '22 19:08 romainmenke

Released in v15

RyanZim avatar Aug 30 '22 22:08 RyanZim