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

Use context.containing_url to track import stack #92

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Conversation

ntkme
Copy link
Member

@ntkme ntkme commented Apr 5, 2024

This is a major rewrite of importer wrapper to track import stack purely with context.containing_url (a feature added in dart-sass 1.68.0), instead of tracking our own stack with indirections via virtual sass files.

Previously both single ImportResult and multi ImportResult arrays are handled with a virtual scss file for stack tracking. For a single ImportResult, it loads in three files: the first virtual file to "push" into the stack, the second actual file, and the third virtual file to "pop" out of the stack. Multi ImportResult arrays would generate an extra virtual sass file, in which each individual ImportResult loads three files the same way as single ImportResult.

Now single ImportResult is handled directly without having any virtual sass files.

This change has many benefits:

  • Robust stack tracking.
    • Fix potentail bugs that native importers like NodePkgImporter breaks stack tracking as they don't go through generated virtual files.
  • Better performance.
    • Reduce compiler round trips by removing of majority of indirections from each single ImportResult.
  • Better source map.
    • Eliminate two virtual sass files per single ImportResult.
    • Create more readable virtual sass files for multi ImportResult arrays, which now import the resolved paths directly rather than import other virtual files.

@ntkme ntkme merged commit ffcdf8c into main Apr 5, 2024
76 checks passed
@ntkme ntkme deleted the containing-url branch April 5, 2024 20:14
@ntkme ntkme mentioned this pull request Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant