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.

Sign up for GitHub

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

Jump to bottom

perf: reuse compiler process when using sass-embedded #1195

Merged
merged 7 commits into from
Apr 9, 2024
Merged

perf: reuse compiler process when using sass-embedded #1195

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecc9206
perf: reuse compiler process when using sass-embedded
renspoesse Mar 28, 2024
5952f5a
refactor: add `modern-compiler` API option and call to `dispose()`
renspoesse Mar 29, 2024
2e8a443
refactor: allow `implementation` to change
renspoesse Mar 29, 2024
b111ffd
test: perform same tests for `modern-compiler` as for `modern`
renspoesse Mar 29, 2024
e75dbde
refactor: use WeakMap
renspoesse Apr 2, 2024
2514e16
test: add test for switching implementation with modern-compiler API
renspoesse Apr 2, 2024
ea88129
style: ran prettier
renspoesse Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,12 +657,12 @@ module.exports = {
Type:

```ts
type api = "legacy" | "modern";
type api = "legacy" | "modern" | "modern-compiler";
```

Default: `"legacy"`

Allows you to switch between `legacy` and `modern` API. You can find more information [here](https://sass-lang.com/documentation/js-api).
Allows you to switch between `legacy` and `modern` API. You can find more information [here](https://sass-lang.com/documentation/js-api). The `modern-compiler` option enables the modern API with support for [Shared Resources](https://github.com/sass/sass/blob/main/accepted/shared-resources.d.ts.md).

> **Warning**
>
Expand Down
7 changes: 4 additions & 3 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ async function loader(content) {
: true;

if (shouldUseWebpackImporter) {
const isModernAPI = options.api === "modern";
const isModernAPI =
options.api === "modern" || options.api === "modern-compiler";

if (!isModernAPI) {
const { includePaths } = sassOptions;
Expand All @@ -65,7 +66,7 @@ async function loader(content) {
let compile;

try {
compile = getCompileFn(implementation, options);
compile = getCompileFn(this, implementation, options);
} catch (error) {
callback(error);
return;
Expand All @@ -74,7 +75,7 @@ async function loader(content) {
let result;

try {
result = await compile(sassOptions, options);
result = await compile(sassOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused argument.

} catch (error) {
// There are situations when the `file`/`span.url` property do not exist
// Modern API
Expand Down
2 changes: 1 addition & 1 deletion src/options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"api": {
"description": "Switch between old and modern API for `sass` (`Dart Sass`) and `Sass Embedded` implementations.",
"link": "https://github.com/webpack-contrib/sass-loader#sassoptions",
"enum": ["legacy", "modern"]
"enum": ["legacy", "modern", "modern-compiler"]
},
"sassOptions": {
"description": "Options for `node-sass` or `sass` (`Dart Sass`) implementation.",
Expand Down
42 changes: 35 additions & 7 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ async function getSassOptions(
};
}

const isModernAPI = loaderOptions.api === "modern";
const isModernAPI =
loaderOptions.api === "modern" || loaderOptions.api === "modern-compiler";
const { resourcePath } = loaderContext;

if (isModernAPI) {
Expand Down Expand Up @@ -650,20 +651,21 @@ function getWebpackImporter(loaderContext, implementation, includePaths) {
}

let nodeSassJobQueue = null;
const sassModernCompilers = {};

/**
* Verifies that the implementation and version of Sass is supported by this loader.
*
* @param {Object} loaderContext
* @param {Object} implementation
* @param {Object} options
* @returns {Function}
*/
function getCompileFn(implementation, options) {
const isNewSass =
implementation.info.includes("dart-sass") ||
implementation.info.includes("sass-embedded");
function getCompileFn(loaderContext, implementation, options) {
const isDartSass = implementation.info.includes("dart-sass");
const isSassEmbedded = implementation.info.includes("sass-embedded");

if (isNewSass) {
if (isDartSass || isSassEmbedded) {
if (options.api === "modern") {
return (sassOptions) => {
const { data, ...rest } = sassOptions;
Expand All @@ -672,6 +674,32 @@ function getCompileFn(implementation, options) {
};
}

if (options.api === "modern-compiler") {
return async (sassOptions) => {
// eslint-disable-next-line no-underscore-dangle
const webpackCompiler = loaderContext._compiler;
const { data, ...rest } = sassOptions;

// Some people can run the loader in a multi-threading way;
// there is no webpack compiler object in such case.
if (webpackCompiler) {
const key = isDartSass ? "dart-sass" : "sass-embedded";
if (!sassModernCompilers[key]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if getCompileFn() can be invoked with different (implementation) options for a single instance of the sass-loader module, but I guess it can? In that case we should maintain a compiler instance for each implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I see, let's use WeakMap with options to prevent such behaviour and memory leaking

// Create a long-running compiler process that can be reused
// for compiling individual files.
const compiler = await implementation.initAsyncCompiler();
webpackCompiler.hooks.shutdown.tap("sass-loader", () => {
compiler.dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my debugging this hook was never called; not sure when it should be. In any case, should we delete sassModernCompilers[key] after disposing? If webpack may reuse the sass-loader module after the shutdown phase, we should, otherwise the compiler won't be recreated on the next run.

Copy link
Member

Choose a reason for hiding this comment

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

After shutdown should nothing happend in theory, weird, it should happend

});
sassModernCompilers[key] = compiler;
}
return sassModernCompilers[key].compileStringAsync(data, rest);
}

return implementation.compileStringAsync(data, rest);
};
}

return (sassOptions) =>
new Promise((resolve, reject) => {
implementation.render(sassOptions, (error, result) => {
Expand All @@ -686,7 +714,7 @@ function getCompileFn(implementation, options) {
});
}

if (options.api === "modern") {
if (options.api === "modern" || options.api === "modern-compiler") {
throw new Error("Modern API is not supported for 'node-sass'");
}

Expand Down
176 changes: 176 additions & 0 deletions test/__snapshots__/additionalData-option.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,34 @@ exports[`additionalData option should use same EOL on all os ('dart-sass', 'mode

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"a {
color: hotpink;
}

body {
color: hotpink;
}"
`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"a {
color: red;
}

body {
color: hotpink;
}"
`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('dart-sass', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('node-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"a {
color: hotpink; }
Expand Down Expand Up @@ -138,6 +166,34 @@ exports[`additionalData option should use same EOL on all os ('sass-embedded', '

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"a {
color: hotpink;
}

body {
color: hotpink;
}"
`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"a {
color: red;
}

body {
color: hotpink;
}"
`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should use same EOL on all os ('sass-embedded', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('dart-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
Expand Down Expand Up @@ -178,6 +234,26 @@ exports[`additionalData option should work as a function ('dart-sass', 'modern'

exports[`additionalData option should work as a function ('dart-sass', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a function ('dart-sass', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('node-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink; }
Expand Down Expand Up @@ -238,6 +314,26 @@ exports[`additionalData option should work as a function ('sass-embedded', 'mode

exports[`additionalData option should work as a function ('sass-embedded', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('dart-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
Expand Down Expand Up @@ -278,6 +374,26 @@ exports[`additionalData option should work as a string ('dart-sass', 'modern' AP

exports[`additionalData option should work as a string ('dart-sass', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a string ('dart-sass', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('node-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink; }
Expand Down Expand Up @@ -338,6 +454,26 @@ exports[`additionalData option should work as a string ('sass-embedded', 'modern

exports[`additionalData option should work as a string ('sass-embedded', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as a string ('sass-embedded', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('dart-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
Expand Down Expand Up @@ -378,6 +514,26 @@ exports[`additionalData option should work as an async function ('dart-sass', 'm

exports[`additionalData option should work as an async function ('dart-sass', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as an async function ('dart-sass', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('node-sass', 'legacy' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink; }
Expand Down Expand Up @@ -437,3 +593,23 @@ exports[`additionalData option should work as an async function ('sass-embedded'
exports[`additionalData option should work as an async function ('sass-embedded', 'modern' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern' API, 'scss' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'sass' syntax): warnings 1`] = `[]`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): css 1`] = `
"body {
color: hotpink;
}"
`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): errors 1`] = `[]`;

exports[`additionalData option should work as an async function ('sass-embedded', 'modern-compiler' API, 'scss' syntax): warnings 1`] = `[]`;
8 changes: 8 additions & 0 deletions test/__snapshots__/implementation-option.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ exports[`implementation option 'dart-sass', 'modern' API: errors 1`] = `[]`;

exports[`implementation option 'dart-sass', 'modern' API: warnings 1`] = `[]`;

exports[`implementation option 'dart-sass', 'modern-compiler' API: errors 1`] = `[]`;

exports[`implementation option 'dart-sass', 'modern-compiler' API: warnings 1`] = `[]`;

exports[`implementation option 'node-sass', 'legacy' API: errors 1`] = `[]`;

exports[`implementation option 'node-sass', 'legacy' API: warnings 1`] = `[]`;
Expand All @@ -24,6 +28,10 @@ exports[`implementation option 'sass-embedded', 'modern' API: errors 1`] = `[]`;

exports[`implementation option 'sass-embedded', 'modern' API: warnings 1`] = `[]`;

exports[`implementation option 'sass-embedded', 'modern-compiler' API: errors 1`] = `[]`;

exports[`implementation option 'sass-embedded', 'modern-compiler' API: warnings 1`] = `[]`;

exports[`implementation option not specify with legacy API: errors 1`] = `[]`;

exports[`implementation option not specify with legacy API: warnings 1`] = `[]`;
Expand Down