Deprecated Sass not allowing modern packaged imports

[john-easci]

Describe the bug

The system uses sass.render(), which has been deprecated for several years now.

I found that it was not possible to use the Node.js package importer (https://sass-lang.com/documentation/js-api/classes/nodepackageimporter/) with the current setup.

Additionally, it uses the Sass async renderer for no good reason, and the async renderer is generally twice as slow as the standard one. It was wrapped in a promise anyway, so I don't think there was a good reason for it.

So, this is the code patch, which is in WorkerWithFallback... basically replacing everything after creating the _internalImporter (which isn't even used here). I also remove the node:path since it is no longer necessary.

I get this is a major component, but its currently using a deprecated API and breaks some very valid use cases. Maybe there's a flag for modern handling? Or maybe somebody can point me to where I can patch this functionality without getting into source?

    // eslint-disable-next-line no-restricted-globals
    const url = require('node:url');
    /**
     * The original internalImporter does not appear to be 
     * compatible with the new interface.  Unsure if it is still
     * needed here, as it appears to be tied to some file.meta bug.
     * 
     * If that's the case -- why not just update the `.url` property?
     */
    const importers = options.importers ?? [];

    // reshape the legacy API so that older code can still use it, but maintain values passed to the 
    // new system.
    const remap: Sass.Options<"sync"> = Object.assign({}, options, {
        importers: importers,
        loadPaths: options.includePaths,
        sourceMap: options.enableSourcemap,
        functions: {} as Record<string, Sass.CustomFunction<"sync">>
    });

    // Legacy importers aren't going to work... this could be problematic for folk.
    delete (remap as any).importer;
    delete (remap as any).includePaths;
    delete (remap as any).enableSourcemap;

    // Set appropriate closure and data.
    let fnCompile = sass.compile;
    let srcItem = options.filename;

    // It looks like there's a big chunk of support for injecting
    // some variable data at the head of the file.  This should work,
    // although that seems an odd thing to do. It could be automatic in the system,
    // which is fine... can just remove this if and make it the 'only'.
    if (data) {
        fnCompile = sass.compileString;
        srcItem = data;
        // quickly reshape this to StringOptions.  This is a bit of a hack.
        (remap as Sass.StringOptions<"sync">).url = url.pathToFileURL(options.filename);
    }

    return new Promise((resolve, reject) => {
        try {
            let out = {
                stats: {
                    includedFiles: [] as string[]
                }
            };
            const res = fnCompile(srcItem, remap);
            out = Object.assign(out, res);

            // rebuild stats.includedFiles, since they're wanted
            // in some downstream process.
            out.stats = {
                includedFiles: res.loadedUrls.map( (item) => {
                    return item.pathname
                })
            }
            resolve(out);
        } catch (e) {
            reject(e);
        }
    });

Reproduction

https://stackblitz.com/edit/vitejs-vite-qttc98?file=package.json

Steps to reproduce

I effed around with this StackBlitz for a while but gave up. It looks like it doesn't support github or bitbucket repositories (even public), so I couldn't finalize the simplified test case.

npm run dev.
open site
font should be Times

System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 165.84 MB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.15.0 - ~/.nvm/versions/node/v18.15.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.15.0/bin/yarn
    npm: 9.5.0 - ~/.nvm/versions/node/v18.15.0/bin/npm
    pnpm: 9.0.6 - ~/.nvm/versions/node/v18.15.0/bin/pnpm
    Watchman: 2022.01.17.00 - /usr/local/bin/watchman
  Browsers:
    Chrome: 124.0.6367.119
    Edge: 124.0.2478.80
    Safari: 17.4.1
  npmPackages:
    @vitejs/plugin-react-swc: ^3.5.0 => 3.6.0
    vite: ^5.2.0 => 5.2.11

Used Package Manager

yarn

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[stackblitz]

Start a new pull request in StackBlitz Codeflow.

• vitejs-vite-qttc98 (Open reproduction)

[john-easci]

And somehow I missed #14689.

It looks like sass-loader (another package, but whatever -- added a top-level option for "modern" (defaulting to legacy) as an overriding option just over 2 years ago (webpack-contrib/sass-loader#774).

This allowed them to not break people -- but also support the newer standards through a separate code tree. That's probably a smarter way to handle this.

[jamesnw]

@john-easci You can use the Node Package Importer in Vite, using the pkgImporter option- https://sass-lang.com/documentation/js-api/interfaces/legacyfileoptions/#pkgImporter

Here's a write-up on how to enable the Node Package Importer in Vite- https://www.oddbird.net/2024/02/22/pkg-importer/#enabling-pkg%3A-urls-in-vite

[john-easci]

@jamesnw Thank you!

I had tried using pkgImporter, but it wasn't working. It turns out that in the modern API, the system does a good job of resolving the source filenames, while the legacy API does not. I tested this by just calling sass.compile() and sass.render from an external file, and while compile() correctly determined path structures, render() did not.

I was able to get it working by adding the appropriate directories to the includePaths. While it isn't ideal (since you need to manage package data in multiple places), it totally works for now.