Skip to content

JS API tests for loading multiple nested paths with renderSync#1910

Merged
Goodwine merged 5 commits intomainfrom
nested-multiple-imports
Jul 1, 2023
Merged

JS API tests for loading multiple nested paths with renderSync#1910
Goodwine merged 5 commits intomainfrom
nested-multiple-imports

Conversation

@Goodwine
Copy link
Member

@Goodwine Goodwine commented May 17, 2023 •
edited
Loading

Issue: sass/dart-sass#1962

[skip sass-embedded]

See: sass/embedded-host-node#226

ntkme
ntkme suggested changes May 17, 2023
View reviewed changes
@Goodwine Goodwine force-pushed the nested-multiple-imports branch 2 times, most recently from d92ed37 to 477dd43 Compare May 17, 2023 23:54
Goodwine
Goodwine commented May 17, 2023
View reviewed changes
@nex3
Copy link
Contributor

nex3 commented May 18, 2023

What parameters are actually passed to the importer in plain Dart Sass versus the embedded host?

@Goodwine
Copy link
Member Author

Goodwine commented May 18, 2023

What parameters are actually passed to the importer in plain Dart Sass versus the embedded host?

I'm not 100% sure if this is what you're referring to, but:

dart-sass importer

console.log
  ## DEBUG: url= ./foo/_a.scss - prev: /home/goodwine/repos/sass-spec/spec/sandbox/9067981249289676/src/index.scss
console.log
  ## DEBUG: url= ./foo/_a.scss - prev: /home/goodwine/repos/sass-spec/spec/sandbox/9067981249289676/src/_includes.scss
console.log
  ## DEBUG: url= ./foo/_b.scss - prev: /home/goodwine/repos/sass-spec/spec/sandbox/9067981249289676/src/_includes.scss

embedded importer

console.log
  ## DEBUG: url= foo/_a.scss - prev: /home/goodwine/repos/sass-spec/spec/sandbox/308212682427188/src/index.scss
console.log
  ## DEBUG: url= file:///home/goodwine/repos/sass-spec/spec/sandbox/308212682427188/src/foo/_b.scss - prev: /home/goodwine/repos/sass-spec/spec/sandbox/308212682427188/src/_includes.scss
<crash>

I looked at the implementations, and dart-sass seems to just call the importer without trying to do much wrapping, but embedded-host-node has some fancier wrapping and I think that's where it goes "wrong"

Like if I implement a "modern" importer using compile() instead of renderSync() I would have to implement a correct canonalize() and I would realize that the input Sass is kinda wrong 🤔

@Goodwine Goodwine requested a review from nex3 May 18, 2023 20:24
@Goodwine Goodwine force-pushed the nested-multiple-imports branch from 477dd43 to f70b6ee Compare June 7, 2023 00:25
@Goodwine Goodwine marked this pull request as ready for review June 7, 2023 18:21
@Goodwine Goodwine mentioned this pull request Jun 7, 2023
Merged
@Goodwine Goodwine force-pushed the nested-multiple-imports branch from 8c77be9 to 52a2b2b Compare June 7, 2023 19:32
@Goodwine Goodwine requested a review from ntkme June 7, 2023 20:28
nex3
nex3 approved these changes Jun 9, 2023
View reviewed changes
@Goodwine Goodwine force-pushed the nested-multiple-imports branch from 28e63e9 to 81deeba Compare June 23, 2023 23:10
@Goodwine @nex3
Update js-api-spec/legacy/importer.node.test.ts
2c961ec 
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
@Goodwine Goodwine merged commit b926940 into main Jul 1, 2023
@Goodwine Goodwine deleted the nested-multiple-imports branch July 1, 2023 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nex3 nex3 nex3 approved these changes

@ntkme ntkme Awaiting requested review from ntkme

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Goodwine @nex3 @ntkme