sass / embedded-host-node

A Node.js library that will communicate with Embedded Dart Sass using the Embedded Sass protocol
MIT License
166 stars 29 forks source link

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

Closed sapphi-red closed 3 weeks ago

sapphi-red commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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: https://github.com/sass/embedded-host-node/pull/261#issuecomment-1870717773

sapphi-red commented 3 weeks ago

Ah, I see.