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

feat: support webpack built-in resolver for modern and modern-compile… #1197

Merged

Conversation

alexander-akait
Copy link
Member

…r API

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

fixes #774

Breaking Changes

No

Additional Info

No

@alexander-akait alexander-akait force-pushed the feat-support-import-for-modern-and-modern-compiler-api branch from e633761 to 4a00573 Compare April 11, 2024 13:49
@alexander-akait alexander-akait merged commit 2265b72 into master Apr 11, 2024
9 checks passed
@alexander-akait alexander-akait deleted the feat-support-import-for-modern-and-modern-compiler-api branch April 11, 2024 15:23
Comment on lines +770 to +779
if (!sassModernCompilers.has(webpackCompiler)) {
sassModernCompilers.set(webpackCompiler, compiler);
webpackCompiler.hooks.shutdown.tap("sass-loader", () => {
compiler.dispose();
});
}
}

return sassModernCompilers
.get(implementation)
.get(webpackCompiler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-akait the sassModernCompilers map should map to implementation, not webpackCompiler. The current modern-compiler API in 14.2.0 is broken and leads to my system becoming unresponsive; I think because of this change.

Copy link
Member Author

@alexander-akait alexander-akait Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renspoesse We can't use implementation as a key, because when you have two compilers with sass-loader (i.e. multi compiler mode) after closing first we close sass compiler, so the second will failed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leads to my system becoming unresponsive

Please provide steps to reproduce

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide steps to reproduce

On a larger project, running webpack in watch mode with modern-compiler. I don't have detailed steps.

because when you have two compilers with sass-loader (i.e. multi compiler mode) after closing first we close sass compiler, so the second will failed

If we do want to use webpackCompiler as the key, this line also needs updating:

if (!sassModernCompilers.has(implementation)) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah, I will fix 👍

On a larger project, running webpack in watch mode with modern-compiler. I don't have detailed steps.

Do you find a problem? I want to investigate your problem

Copy link
Contributor

@renspoesse renspoesse Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with the latest version and the issue still exists. I ran the debugger and stepped through sass-loader, and it looks like webpackCompiler doesn't maintain a stable reference across invocations for different Sass files; so sassModernCompilers.has(implementation) still isn't true and we keep creating a new Dart process for each file.

Copy link
Member Author

@alexander-akait alexander-akait Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with the latest version and the issue still exists. I ran the debugger and stepped through sass-loader, and it looks like webpackCompiler doesn't maintain a stable reference across invocations for different Sass files; so sassModernCompilers.has(implementation) still isn't true and we keep creating a new Dart process for each file.

this is not possible because we are creating the only one compiler per configuration

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a reproducible test repo to undestand your problem, because compiler lives from beginning to end of your watch/run/serve

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating mini-css-extract-plugin from 1.6.0 to 2.9.0 resolved the issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renspoesse Great ⭐

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.

Option to use the DartVM executable rather than the pure-js version of dart-sass.
2 participants