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

Export default importer in the JavaScript API #3655

Closed
lachlanmcdonald opened this issue Aug 15, 2023 · 6 comments
Closed

Export default importer in the JavaScript API #3655

lachlanmcdonald opened this issue Aug 15, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@lachlanmcdonald
Copy link

lachlanmcdonald commented Aug 15, 2023

A quality-of-life suggestion (or work-around until #3247) — when authoring a custom importer it would be nice to be able to drop-in the existing default importer as a fall-back:

For instance, this would be functionally identical to:

const { compileString } = require('sass');
const { defaultImporter } = require('sass'); // Proposed

compileString('@import "ui";', {
  importer: defaultImporter
);

But would allow for the following structure:

const { compileString } = require('sass');
const { defaultImporter } = require('sass'); // Proposed

compileString('@import "blob:ui";', {
  importer: {
    canonicalize(url, options) {
      const u = new URL(url);

      if (u.protocol === 'blob:') {
        // Do something 
      } else {
        // Pass-through
        return defaultImporter.canonicalize(url, options);
      }
    },
    load: defaultImporter.load,
  }
});

This would allow an author of an Importer to handle only their specific use-case, and not worry about re-implementing the file-naming edge-cases already supported by Sass.


I assume importers would be a conceptually better place for this kind of behaviour, as it is intended to handle fall-backs once Sass has had a first pass at it. However, this suggestion arises as the importer option resolves a relative URL to the parent file, but importers does not. Therefore, to create a custom importer that is relative to the parent file, only importer option can be used.

As an aside, in the above example, blob: isn't a great fit as a different protocol would not resolve to a relative URL anyway, but it's just for illustrative purposes.

@Goodwine
Copy link
Member

It seems like what you are looking for is writing an importer where you only implement canonicalize(), but not load(). If so, I believe you are looking for the FileImporter.

Also note that you can "pass-through" a URL by having the function return null which lets subsequent importers deal with the import.

https://sass-lang.com/documentation/js-api/interfaces/fileimporter/#findFileUrl.findFileUrl-1

“If this importer doesn't recognize the URL, it should return null instead to allow other importers or load paths to handle it.”

https://sass-lang.com/documentation/js-api/interfaces/importer/#canonicalize.canonicalize-1

“An absolute URL if this importer recognizes the url, or null if it doesn't. If this returns null, other importers or load paths may handle the load.”

So you want something like...

compileString('@import "blob:ui";', {
  importers: [{ // importers, not importer
    canonicalize(url, options) {
      const u = new URL(url);
      // Handle blobs only, let the rest URLs be loaded by
      // other importers
      return u.protocol === 'blob:' ? url : null;
    },
    load(url) {
      // ... This load() will only work for "blob:" URLs
    },
  }, /* ... other importers? ... */]
});

@lachlanmcdonald
Copy link
Author

lachlanmcdonald commented Aug 15, 2023

Hi @Goodwine, thanks for the quick response. I think there has been some confusion — the challenge here is that only importer will resolve relative URLs, importers does not. So neither importers or FileImporter are suitable in the circumstance where someone would want to have the relative path resolution but not re-invent the wheel on handling import edge-cases.

(The example was to illustrate how the default importer can be used as a drop-in when writing a custom importer, but not the actual use-case. I can see how that might have caused some confusion.)

@Goodwine
Copy link
Member

Even with importer, wouldn't returning null in canonicalize() achieve what you're looking for?

@lachlanmcdonald
Copy link
Author

lachlanmcdonald commented Aug 16, 2023

Unless I'm missing the correct usage of canonicalize() — If returning null in the importer option were to fall back to the default importer, then these two snippets should be functionally identical:

// Compiles successfully
compileString('@import "partial";', {
  url: pathToFileURL(path.join(__dirname, 'style.scss')),
});
// Fails to handle the import
compileString('@import "partial";', {
  url: pathToFileURL(path.join(__dirname, 'style.scss')),
  importer: {
    canonicalize(url) { return null; },
    load(url) { return null; },
  }
});

However, the moment the importer option is defined, it appears to override the default importer. I believe this is why #3247 exists, importer resolves paths relative to the file attemping to import, but importers does not.

When using importer, we would be required to implement the checks for directories with index files (i.e. partial/index.scss), partials (_a.scss and a.scss), and different syntax's (.sass and .scss) and keep it aligned with Sass' implementation.

For instance, in this contrived example where @ denotes a path to a JSON file relative to the scss file importing it:

const result = compileString(`
  @import "@peach";
  @import "partial";
`, {
  url: pathToFileURL(path.join(__dirname, 'style.scss')),
  importer: {
    canonicalize(url) {
      const baseName = path.basename(url);

      if (baseName.startsWith('@')) {
        const filePath = fileURLToPath(url);
        return new URL("color:" + filePath);
      }

      return null;
    },
    load(url) {
      if (url.protocol === 'color:') {
        const parentDir = path.dirname(url.pathname);
        const baseName = path.basename(url.pathname, path.extname(url.pathname)).slice(1);
        const jsonPath = path.join(parentDir, baseName + ".json");

        const data = JSON.parse(fs.readFileSync(jsonPath, "utf8"));

        return {
          contents: `body { background-color: ${ data.color }; }`,
          syntax: 'scss'
        };
      }
      return null;
    }
  }
});

Would successfully handle @import "@peach" but fail on @import "partial" as we haven't first re-implemented the expected import behaviours.

@Goodwine
Copy link
Member

Got it, thanks for the additional explanation, I'll reopen so @nex3 can see it and maybe comment additionally to this.

Then, my take is that exporting defaultImporter through the JS API as a workaround opens the door to introducing breaking changes because if it's a workaround it should go away, but there would be users now depending on it. Since the #3247 proposal is marked as "Accepted", I believe that waiting on its implementation is probably the best course of action. But I'll leave it to Natalie to comment on this when she returns

@Goodwine Goodwine reopened this Aug 16, 2023
@Goodwine Goodwine added the enhancement New feature or request label Aug 16, 2023
@nex3
Copy link
Contributor

nex3 commented Aug 21, 2023

We do provide access to the internal FilesystemImporter from Dart, but that's trickier from JS—the embedded host doesn't have direct access to the importer logic (which is written in Dart), so it would effectively have to reimplement it from scratch in pure JS. (That's why we added support for a separate class of filesystem importers in the JS API in the first place.)

That said, for the use-case you describe here of effectively adding a new file extension that can be loaded as Sass (such as .json), it's probably a good idea to support Sass-style resolution for that file extension as well (so authors can write @use "peach" and have that load peach/_index.json or whatever). This would require implementing a custom variant of Sass's resolution logic anyway, so I'm not sure it's worth providing it as a built-in.

@nex3 nex3 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants