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

Using legacy APIs with importers triggers @import deprecation warning #340

Closed
sapphi-red opened this issue Oct 21, 2024 · 6 comments · Fixed by #342
Closed

Using legacy APIs with importers triggers @import deprecation warning #340

sapphi-red opened this issue Oct 21, 2024 · 6 comments · Fixed by #342
Assignees
Labels
bug Something isn't working

Comments

@sapphi-red
Copy link

Even if the code doesn't contain @import, the @import deprecation warning appears.

Deprecation Warning: Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.

More info and automated migrator: https://sass-lang.com/d/import

  ╷
5 │ ;@import "sass-embedded-legacy-load-done:0";
  │          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵

While I can set silenceDeprecations, it's a bit confusing.

Reproduction:

  1. clone https://github.com/sapphi-red-repros/sass-embedded-legacy-importer-triggers-import-deprecated-warning
  2. run npm i
  3. run node index.js
@ntkme
Copy link
Contributor

ntkme commented Oct 21, 2024

This is somewhat expected, due to the way the legacy API in the embedded-host-node is emulated with modern API.

#309 might fix this but we’ve decided that it’s not worth to do such a major rewrite given that the legacy API will be removed soon in 2.0.0.

@sapphi-red
Copy link
Author

Is it possible to filter out the deprecation warning containing sass-embedded-legacy-load-done by default? I think that would suffice for my case.

@ntkme
Copy link
Contributor

ntkme commented Oct 22, 2024

Is it possible to filter out the deprecation warning containing sass-embedded-legacy-load-done by default?

It's possible. However, filtering out the warning internally is only a partial workaround for the default config. When a user choose to opt-in with fatalDeprecations: ['import'], it will crash the compilation.

Here is a patch:

diff --git a/lib/src/compiler/utils.ts b/lib/src/compiler/utils.ts
index f04f03f..ee94263 100644
--- a/lib/src/compiler/utils.ts
+++ b/lib/src/compiler/utils.ts
@@ -168,6 +168,13 @@ export function handleLogEvent(
     ? deprecations[event.deprecationType]
     : null;
 
+  if (
+    deprecationType === deprecations.import &&
+    span?.text.startsWith('"sass-embedded-legacy-load-done:')
+  ) {
+    return;
+  }
+
   if (event.type === proto.LogEventType.DEBUG) {
     if (options?.logger?.debug) {
       options.logger.debug(message, {

Although this patch silences the deprecation for internal @import used by emulated legacy API, the follow code will still crash unexpectedly:

sass.renderSync({ data: '@use "test"', fatalDeprecations: ['import'], importer: [
  function(url, _) {
    if (url != 'test') return null;

    return {
      contents: 'a {b: c}'
    };
  }]
});

@sapphi-red
Copy link
Author

Although this patch silences the deprecation for internal @import used by emulated legacy API, the follow code will still crash unexpectedly.

I guess that can be workaround by converting it to a normal deprecation and throwing on the host side.

  const wasImportIncludedInFatalDeprecations = fatalDeprecations.includes('import')
  // also remove `import` from options.fatalDeprecations

  // ---

  if (deprecationType === deprecations.import) {
    if (span?.text.startsWith('"sass-embedded-legacy-load-done:')) {
      return;
    }
    if (wasImportIncludedInFatalDeprecations) {
      throw new Error('@import fatal deprecation')
    }
  }

@ntkme
Copy link
Contributor

ntkme commented Oct 22, 2024

That is certainly possible, but unfortunately not as straightforward as you would think.

The main problem is that the event dispatcher in a synchronous compiler currently dispatches log event asynchronously, meaning anything thrown in logger will just become unhandled exception that is not catchable. See original discussion here: #261 (comment)

@sapphi-red
Copy link
Author

Ah, I see.

@nex3 nex3 self-assigned this Oct 22, 2024
@nex3 nex3 added the bug Something isn't working label Oct 22, 2024
nex3 added a commit that referenced this issue Oct 22, 2024
This unfortunately may make error reports look a little odd for errors
on the first line of SCSS files loaded via the legacy importer and
it'll give indented-syntax errors an off-by-one error, but there's an
easy fix: stop using the legacy API.

Closes #340
nex3 added a commit that referenced this issue Oct 29, 2024
This unfortunately may make error reports look a little odd for errors
on the first line of SCSS files loaded via the legacy importer and
it'll give indented-syntax errors an off-by-one error, but there's an
easy fix: stop using the legacy API.

Closes #340

Co-authored-by: Jennifer Thakar <jathak@google.com>
@nex3 nex3 closed this as completed in #342 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants