[Package Importer] Specs #1952
[Package Importer] Specs #1952
Conversation
…ore render called
…ec into feature.package-importer
| @@ -35,7 +35,29 @@ it('an empty object means an empty file', () => | |||
|
|
|||
| describe('import precedence:', () => { | |||
| describe('in sandbox dir', () => { | |||
| it('relative file is #1', () => | |||
| it('packageImporter:node is #1', () => | |||
There was a problem hiding this comment.
This isn't relevant to the import precedence that this describe() is testing. These tests verify what happens when there are multiple possible interpretations of the same load. Because the package importer is always absolute (since it has the pkg: scheme), it's never ambiguous with relative, load path, or working-directory loads.
There was a problem hiding this comment.
That makes sense. I've removed this test here. I added this as the spec specifies that Package Importers are resolved first for the legacy API. Do we need to test that, or perhaps, is there really a way to test that, as there can't be conflicts?
….package-importer
….package-importer
There was a problem hiding this comment.
@nex3 I needed to make this change after merging Shared Resources into node-embedded-host, as all async tests were failing. I think this is correct now (the callback can be async, so we need to await it, so that the finally doesn't happen before the callback resolves).
However, it seemed worth flagging with you because the async dart-sass tests did not fail, and the node-embedded-host tests did not fail until after merging in Shared Resources, and we weren't able to track down any cause for this difference.
There was a problem hiding this comment.
It could be that functions with FutureOr<T> return type in dart is not really an async function in JS, but a sync function that may return a Promise or T. This allows the some synchronous code path to be dispatched synchronously, and only code inside the returned Promise is dispatched asynchronously, therefore may lead to different execution order than how a true asynchronous function is dispatched.
js-api-spec/importer.test.ts
Outdated
| compileString('@use "pkg:foo";', { | ||
| importers: [new NodePackageImporter()], | ||
| }) |
There was a problem hiding this comment.
This should just test new NodePackageImporter() without passing it to compileString() now.
There was a problem hiding this comment.
Addressed in oddbird@b18ff2c
sass/sass#2739
Blocked until proposal is accepted.
[skip dart-sass]
[skip sass-embedded]