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

Provide access to the containing stylesheet's URL for some loads #3247

Closed
5 tasks done
nex3 opened this issue Jan 14, 2022 · 9 comments
Closed
5 tasks done

Provide access to the containing stylesheet's URL for some loads #3247

nex3 opened this issue Jan 14, 2022 · 9 comments
Labels
enhancement New feature or request JS API About the shared JS API proposal accepted A full proposal for this feature has been marked accepted

Comments

@nex3
Copy link
Contributor

nex3 commented Jan 14, 2022


In webpack-contrib/sass-loader#774, @alexander-akait requested some way to access the canonical URL of the stylesheet that contains a @use or @import when canonicalizing a URL in a custom importer.

The motivating use-case is supporting Node-style name resolution where different node_modules directories are used for dependencies in different packages. For example, to match Node's dependency resolution logic, @import "bar" in node_modules/foo/style.scss should prefer node_modules/foo/node_modules/bar/_index.scss to node_modules/bar/_index.scss.

Invariants

We need to support this behavior without making it easy for importers to violate the invariants I described in this comment:

  1. There must be a one-to-one mapping between (absolute) canonical URLs and stylesheets. This means that even when a user loads a stylesheet using a relative URL, that stylesheet must have an absolute canonical URL associated with it and loading that canonical URL must return the same stylesheet. This means that any stylesheet can always be loaded using its canonical URL.

  2. Relative URLs are resolved like paths, so for example within scheme:a/b/c.scss the URL ../d should always be resolved to scheme:a/d.

  3. Loads relative to the current stylesheet always take precedence over loads from the load path (including virtual load paths exposed by importers), so if scheme:a/b/x.scss exists then @use "x" within scheme:a/b/c.scss will always load.

Risks

Providing access to the containing URL puts these invariants at risk in two ways:

  1. Access to the containing URL in addition to a canonical URL makes it possible for importer authors to handle the same canonical URL differently depending in different contexts, violating invariant (1).

  2. It's likely that importer authors familiar with the legacy API will incorrectly assume that any containing URL that exists is the best way to handle relative loads, since the only way to do so in the legacy API was to manually resolve them relative to the prev parameter. Doing so will almost certainly lead to violations of invariant (3) and possibly (2).

Mitigations

We can mitigate these risks by making the containing URL available only under certain circumstances. Offhand, I see three possible restrictions we could put in place:

  1. Don't provide the containing URL when the canonicalize() function is being called for a relative load. This mitigates risk (2) by ensuring that all relative URL resolution is handled by the compiler by default. The importer will be invoked with an absolute URL and no containing URL first for each relative load, which will break for any importers that naïvely try to use the containing URL in all cases.

    This has several drawbacks. First, a badly-behaved importer could work around this by returning null for all relative loads and then manually resolving relative URLs as part of its load path resolution, thus continuing to violate invariant (3). Second, this provides no protection against risk (1) since the stylesheet author may still directly load a canonical URL.

  2. Don't provide the containing URL when the canonicalize() function is being called for any absolute URL. Since relative loads always pass absolute URLs to their importers, this is a superset of mitigation (1). In addition, it protects against risk (1) by ensuring that all absolute URLs (which are a superset of canonical URLs) are canonicalized without regard to context.

    However, this doesn't do anything to address a poorly-behaved importer's ability to continue to violate invariant (3). Worse, it limits the functionality of importers that use a custom URL scheme for non-canonical URLs. For example, if we choose to support Ship a built-in package importer #2739 by claiming the pkg: scheme as a "built-in package importer", implementations of this scheme wouldn't be able to do context-sensitive resolution. This would make the scheme useless for supporting Node-style resolution, a core use-case. Given that we want to encourage users to use URL schemes rather than relative URLs, this is a blocking limitation.

  3. Allow importers to opt into receiving the containing URL by agreeing to additional restrictions. This is motivated by the observation that the primary drawback to mitigation (2) is that we effectively want the same importer to be able to support multiple different URL schemes, some that are canonical and some that are not. To allow for context-dependent canonicalization of non-canonical absolute URLs, we could for example allow an importer to define a nonCanonicalScheme: string field which indicates a URL scheme that the importer supports for canonicalize() but that it will never return from canonicalize(). Then we could provide the containing URL specifically for resolutions of URLs with that scheme, and throw an error if the importer returns a URL of the same scheme.

    However, this API would still involve passing the containing URL to all relative loads, so it still doesn't fully eliminate risk (2). It's worth considering if there's a different restriction we could impose that would eliminate both risk (1) and risk (2).

@alexander-akait
Copy link

Loads relative to the current stylesheet always take precedence over loads from the load path (including virtual load paths exposed by importers), so if scheme:a/b/x.scss exists then @use "x" within scheme:a/b/c.scss will always load.

I think it can be problem, aliasing is good approach to replace variables, for example I can have default variables and built multiple themes - dark/ligth/normal (default). I can describe more cases if you need.

Access to the containing URL in addition to a canonical URL makes it possible for importer authors to handle the same canonical URL differently depending in different contexts, violating invariant (1).

Sass can output warning in this case, I think you already have loaded map

It's likely that importer authors familiar with the legacy API will incorrectly assume that any containing URL that exists is the best way to handle relative loads, since the only way to do so in the legacy API was to manually resolve them relative to the prev parameter. Doing so will almost certainly lead to violations of invariant (3) and possibly (2).

In theory sass can handle it too, you have original module, and request, so sass, I think, also can generate warning here

Developers don't like warnings and will try to fix it, if somebody ignore warnings, it means take risks on self

@nex3
Copy link
Contributor Author

nex3 commented Jan 14, 2022

Loads relative to the current stylesheet always take precedence over loads from the load path (including virtual load paths exposed by importers), so if scheme:a/b/x.scss exists then @use "x" within scheme:a/b/c.scss will always load.

I think it can be problem, aliasing is good approach to replace variables, for example I can have default variables and built multiple themes - dark/ligth/normal (default). I can describe more cases if you need.

This invariant is non-negotiable, I'm afraid. It's an important part of our philosophy of locality. Aliases are totally legit for global loads where the user is relying on a package being available somehow without explicitly indicating how, but for local loads where a local file exists we don't want to overwrite the author's assumption about what will happen. (This also ties into our philosophy of "Sass isn't for making stylesheets do things their authors didn't expect them to do".)

Access to the containing URL in addition to a canonical URL makes it possible for importer authors to handle the same canonical URL differently depending in different contexts, violating invariant (1).

Sass can output warning in this case, I think you already have loaded map

It's likely that importer authors familiar with the legacy API will incorrectly assume that any containing URL that exists is the best way to handle relative loads, since the only way to do so in the legacy API was to manually resolve them relative to the prev parameter. Doing so will almost certainly lead to violations of invariant (3) and possibly (2).

In theory sass can handle it too, you have original module, and request, so sass, I think, also can generate warning here

We'd probably choose to error rather than warn if we can reliably detect these, but I'm not sure how we'd be able to do so.

@alexander-akait
Copy link

This invariant is non-negotiable, I'm afraid. It's an important part of our philosophy of locality. Aliases are totally legit for global loads where the user is relying on a package being available somehow without explicitly indicating how, but for local loads where a local file exists we don't want to overwrite the author's assumption about what will happen. (This also ties into our philosophy of "Sass isn't for making stylesheets do things their authors didn't expect them to do".)

Then I have a question, but how should developers deal with this in using new API, this is quite a common usage pattern, if we say that now this is wrong, I think we should provide an example of how to do it right, thanks for feedback

@nex3
Copy link
Contributor Author

nex3 commented Jan 18, 2022

After talking this out with the team, we realized that the proposed restriction mitigation (3) actually does solve risk (2): if the importer specifies a set of URL schemes it will never return from canonicalize, then relative loads can't use those URL schemes because relative loads are always resolved relative to a canonical URL. So that's probably the best way to move forward.


@alexander-akait

Then I have a question, but how should developers deal with this in using new API, this is quite a common usage pattern, if we say that now this is wrong, I think we should provide an example of how to do it right, thanks for feedback

I think we may be talking about different things. To be clear, if someone write @import "~bootstrap/fonts", you'll still be able to resolve that differently based on where the @import appears—that's what this issue is all about. What you can't do is hijack the resolution for something like @import "src/helpers" from foo/index.scss when foo/src/_helpers.scss exists. I doubt many people are relying on that behavior though!

@alexander-akait
Copy link

Some of developers use it for providing variables for different context and building different versions, for example developer can resolve file with grid include or file without include include, so size will be different, but we can go ahead and see feedback after we implement built-in importer

@stof
Copy link
Contributor

stof commented Sep 1, 2022

@nex3 is there any plan to turn this idea into an actual proposal ?
As I'm started to work on the importer-part of the scssphp 2.0 architecture, I'm wondering whether I should take those ideas into account.

@nex3
Copy link
Contributor Author

nex3 commented Sep 2, 2022

Yes, I definitely want to do this at some point to help sass-loader out—I just haven't had the time yet.

alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enchanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 13, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to angular/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.
alan-agius4 added a commit to angular/angular-cli that referenced this issue Oct 14, 2022
…n Sass when using Yarn PNP

Enhanced resolver is unable to resolve transitive dependencies in Sass when using Yarn PNP. The main reason for this is that Sass doesn't provide context on which file is requesting the module. See: sass/sass#3247.

As a workaround for this we store previously resolved paths and when a new request comes in we try to resolve this from the previously resolved files if we are unable to resolve the request from the workspace root.

(cherry picked from commit c49f1ee)
@nex3
Copy link
Contributor Author

nex3 commented Apr 19, 2023

I finally have some time to work on this. @alexander-akait, just to confirm, moving forward with Mitigation 3 should work for webpack?

@nex3
Copy link
Contributor Author

nex3 commented Apr 27, 2023

I'm giving this a month for public comment, since it potentially affects a lot of plugin authors and there's a lot of nuance to the choice of when to make the URL available.

nex3 added a commit that referenced this issue Jun 21, 2023
nex3 added a commit that referenced this issue Jun 21, 2023
@Goodwine Goodwine added the proposal accepted A full proposal for this feature has been marked accepted label Aug 16, 2023
nex3 added a commit that referenced this issue Sep 15, 2023
nex3 added a commit that referenced this issue Sep 15, 2023
nex3 added a commit that referenced this issue Sep 15, 2023
@nex3 nex3 closed this as completed Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request JS API About the shared JS API proposal accepted A full proposal for this feature has been marked accepted
Projects
None yet
Development

No branches or pull requests

4 participants