Skip to content
This repository has been archived by the owner on May 15, 2023. It is now read-only.

Treat null importer as noOp if url is not a file: url #83

Merged
merged 4 commits into from Apr 12, 2022
Merged

Treat null importer as noOp if url is not a file: url #83

merged 4 commits into from Apr 12, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Apr 3, 2022

@nex3
Copy link
Contributor

nex3 commented Apr 6, 2022

I think the desirable behavior here, to match the JS API spec as updated in sass/sass#3278, is to set a NoOpImporter only if url isn't a file: URL, and to use a filesystem importer otherwise.

@ntkme
Copy link
Contributor Author

ntkme commented Apr 6, 2022

A relative load's URL is first resolved relative to [[url]], then resolved to a file on disk if it's a file:// URL. If it can't be resolved to a file on disk, it's then passed to [[importers]] and [[loadPaths]].

I think the language I picked matches the current behavior: the js-api (non-embedded) only checks if url is null or not to set it to NoOpImporter or FileSystemImporter. This change and sass/embedded-host-node#124 together makes the embedded host consistent with non-embedded js-api.

Today (non-embedded js-api):

  • If url is null -> NoOpImporter
  • If url is non-null -> FileSystemImporter

If I understand your proposal correctly:

  • If url is null -> NoOpImporter
  • If url is non-file url -> NoOpImporter
  • If url is file url -> FileSystemImporter

@nex3 Is that what you mean? In that case we also need to update the non-embedded js-api.

@ntkme ntkme changed the title Treat null importer as noOp Treat null importer as noOp if url is not a file: url Apr 6, 2022
nex3 added a commit that referenced this pull request Apr 7, 2022
Since this provides a substantial amount of the Node embedded
implementation, it's sometimes necessary to test that updates continue
to pass (or begin to pass) the JS API tests, as for example in #83.
nex3 added a commit that referenced this pull request Apr 8, 2022
Since this provides a substantial amount of the Node embedded
implementation, it's sometimes necessary to test that updates continue
to pass (or begin to pass) the JS API tests, as for example in #83.
nex3 added a commit that referenced this pull request Apr 8, 2022
Since this provides a substantial amount of the Node embedded
implementation, it's sometimes necessary to test that updates continue
to pass (or begin to pass) the JS API tests, as for example in #83.
@nex3
Copy link
Contributor

nex3 commented Apr 8, 2022

@ntkme Looks like this is breaking some legacy importer tests, can you take a look?

@ntkme
Copy link
Contributor Author

ntkme commented Apr 9, 2022

@nex3 Looks like we need sass/embedded-host-node#128. The legacy can resolve from working directory without filename set (meaning no file: url). In other words, it always needs a FileSystemImporter.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imports shall not be resolved relative to working directory of dart-sass-embedded process
2 participants