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

Improper handling of Node.js importers that use custom URL schemes #1138

Closed
sebakerckhof opened this issue Nov 9, 2020 · 7 comments · Fixed by #1434
Closed

Improper handling of Node.js importers that use custom URL schemes #1138

sebakerckhof opened this issue Nov 9, 2020 · 7 comments · Fixed by #1434
Assignees
Labels
bug JavaScript Issues particular to the Node.js distribution

Comments

@sebakerckhof
Copy link

In #1100 it was suggested that our custom importer uses URL's like:

@import 'meteor:somepackage/somefile'

This works to some extend (at least since sass 1.26.0 and higher), but only if we can successfully resolve and return the contents.
However, in the case where either an error is thrown, or the contents does not exist (and thus we return null), then dart-sass expects the url to have a file scheme. And dart-sass would throw something like:

Invalid argument(s): Uri meteor:somepackage/somefile must have scheme 'file:'
@sebakerckhof sebakerckhof changed the title Support for Support for non-file url's Nov 9, 2020
@jathak
Copy link
Member

jathak commented Nov 9, 2020

I'm a bit confused by what you're asking for here.

What would you expect Sass to do in this situation? It doesn't have any concept of meteor: URLs; it's the custom importer's job to handle those, so if it doesn't, Sass can't find the file in question, so it throws an error. Is it just about what the error message should say?

@sebakerckhof
Copy link
Author

Ok, I see how I could have worded this better.

Of course, I don't expect sass to know how to handle meteor: urls.
However, if I read the documentation here: https://sass-lang.com/documentation/js-api#importer , it only mentions another importer is going to be tried when an importer returns null (meaning import not found)

So in the case when an importer throws an error, I would expect to see that error message appear. And this works sometimes, like here:
https://gist.github.com/sebakerckhof/4f42ff22554439934f631a2b3a4f3d12

But if an error is thrown deeper down the tree, it gives me the error about the file scheme, e.g.:
https://gist.github.com/sebakerckhof/05bc122d4c97e2da9571e605ad54cbb1

And the behavior when I return null is similar. E.g. here I would get a file not found error:
https://gist.github.com/sebakerckhof/1d295b78301534a1ed96b39d32faf1c9
But here I would get a file scheme error:
https://gist.github.com/sebakerckhof/2cd3644668b5802f8cedffcd7ad48701

@jathak
Copy link
Member

jathak commented Nov 9, 2020

Okay, I see. This is definitely a bug then. We'll take a look.

@jathak jathak added the bug label Nov 9, 2020
@jathak jathak changed the title Support for non-file url's Improper handling of nested dependencies with non-file schemes Nov 9, 2020
@nex3
Copy link
Contributor

nex3 commented Nov 10, 2020

To be clear: are you using the Node.js importer API, or the Dart importer API?

@jathak jathak changed the title Improper handling of nested dependencies with non-file schemes Improper handling of Node.js importers that use custom URL schemes Nov 10, 2020
@jathak
Copy link
Member

jathak commented Nov 10, 2020

I checked and confirmed that this only occurs with the Node API (updated the title to match)

@nex3
Copy link
Contributor

nex3 commented Aug 10, 2021

Sorry to take so long to circle back to this! I'm working on it now.

nex3 added a commit that referenced this issue Aug 10, 2021
When an error occurred in a stylesheet loaded by a custom importer
with a custom URL scheme, the Node error wrapper tried to convert that
URL into a path and ended up crashing.

Closes #1138
nex3 added a commit that referenced this issue Aug 11, 2021
…1434)

When an error occurred in a stylesheet loaded by a custom importer
with a custom URL scheme, the Node error wrapper tried to convert that
URL into a path and ended up crashing.

Closes #1138
@hosseinghs
Copy link

How can I fix this problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug JavaScript Issues particular to the Node.js distribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants