feat(turbopack): basic sass-loader support #4985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
|
✅ This change can build |
|
Linux Benchmark for ed89e67
Click to view full benchmark
|
MacOS Benchmark for ed89e67
Click to view full benchmark
|
jridgewell
reviewed
May 18, 2023
Comment on lines
+70
to
+74
| for (let i = 0; i < errorStack.length; i++) { | ||
| if (errorStack[i].includes(flag)) { | ||
| errorStack.length = i; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this missing a break statement?
There was a problem hiding this comment.
Same goes for https://github.com/vercel/turbo/pull/4985/files#r1197370422, could cut it but thought it'd better to preserve these logics as much as same.
| * @param {string} stack stack trace | ||
| * @returns {string} stack trace without the loader execution flag included | ||
| */ | ||
| const cutOffLoaderExecution = (stack) => cutOffByFlag(stack, loaderFlag); |
There was a problem hiding this comment.
Can you explain why this is needed?
There was a problem hiding this comment.
These are copy-paste from what webpack does for the logging with its levels. We could not do it, but tried to mimic its behavior as much.
There was a problem hiding this comment.
Linux Benchmark for 2a6680dClick to view benchmark
|
MacOS Benchmark for 2a6680d
Click to view full benchmark
|
jridgewell
approved these changes
May 18, 2023
| rootContext: contextDir, | ||
| getOptions() { | ||
| const entry = this.loaders[this.loaderIndex]; | ||
| return entry.options && typeof entry.options === "object" | ||
| ? entry.options | ||
| : {}; | ||
| }, | ||
| getResolve: () => ({ | ||
| // [TODO] this is incomplete |
There was a problem hiding this comment.
We should have a __turbopack_require_context__.resolve() which is our module runtime's resolver.
kwonoj
added
the
pr: automerge
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Linux Benchmark for 7905ffe
Click to view full benchmark
|
Windows Benchmark for 7905ffe
Click to view full benchmark
|
MacOS Benchmark for 7905ffe
Click to view full benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
WEB-654.
This is pairing PR from next.js vercel/next.js#49882 (not a breaking change though) to have necessary support to run sass-loader. Mainly, it adds near-dummy context in
webpack-loadersas well as assigning specific module types for the scss / sass.I'm not entirely in favor of having internal custom module type logic in webpack loaders setup by extension - however next.config.js itself doesn't have enough information other than extension + loader to determine module type. Something would like to address.