thgh / rollup-plugin-scss

Rollup and compile multiple .scss, .sass and .css imports
MIT License
134 stars 47 forks source link

Handle node module duplicate filename import resolution #111

Open andygout opened 1 month ago

andygout commented 1 month ago

Description

This PR addresses an issue that arises from the following conditions:

App directory structure

├── node_modules
│     ├── package-name
│           ├── main.js
│           ├── main.scss
├── src
│     ├── client
│           ├── stylesheets
│                   ├── index.scss
└── rollup.config.js

rollup.config.js

import scss from 'rollup-plugin-scss';
import * as sass from 'sass';

export default {
    input: 'src/client/stylesheets/index.scss',
    output: {
        dir: 'public'
    },
    plugins: [
        scss({
            fileName: 'main.css',
            failOnError: true,
            sass
        })
    ]
};

index.scss

…
@import 'package-name/main';
…

Running rollup --config results in:

[!] (plugin scss) Error: Can't find stylesheet to import.
   ╷
17 │ @import 'package-name/main';
   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  stdin 17:9  root stylesheet

The values at the below lines are:

https://github.com/thgh/rollup-plugin-scss/blob/dd41d2d7a4ce924a0f57e7e59cf9c7b0edd9e146/index.ts#L138-L140

require.resolve() seemingly prioritises files with a .js extension over files with a .scss extension that this plugin will be interested in. It is not treating file extensions alphabetically: the same thing happens if main.css is added alongside main.js and main.scss.

The changes applied by this PR enable the process to make subsequent require.resolve() calls to try and resolve a file with a .css, .scss, or .sass file extension.

Actual use case

I am migrating an app (dramatis-ssr) to native ECMAScript modules and in the process am switching from Webpack to Rollup — branch.

The app uses a package called o-autocomplete (GitHub repo) from a monorepo called origami.

o-atuocomplete includes a main.js and main.scss file.

Considerations

Could require.resolve() take an argument that specifies a file extension?

This would mean that the first call to require.resolve() could specify that it is only looking for files with a .css, .scss, or .sass, and consequently avoid the risk of resolving with a duplicate-named file with a .js extension, thereby preventing the need for subsequent additional require.resolve() calls.

This feature was requested in nodejs/node GitHub issue: add ext option in require.resolve.

In that issue, reference is made to require.extensions, which was added in Node.js v0.3.0 and deprecated since v0.10.6, and whose documentation states:

Avoid using require.extensions. Use could cause subtle bugs and resolving the extensions gets slower with each registered extension.

Admittedly, the changes proposed in this PR result in multiple to attempts to resolve the same import, but only in the scenario of there being a duplicate-named file with a .js extension, which feels more anomalous than common.

The issue also referenced the resolve package but I was wary that it may include different behaviour to require.resolve(). I attempted a direct switch and it seems that the paths option requires different values, indicating that more substantial changes would be required, which it felt best to avoid.

Could the allowedExtensions value be derived from the options.include value (or its default)?

If the options.include value(s) were exclusively globs (as the default value is — ['/**/*.css', '/**/*.scss', '/**/*.sass']) then yes, as the file extensions could be extracted from those strings via String.prototype.match().

In fact, a feature was added to Node.js v22.5.0: path.matchesGlob(), which determines if path matches the pattern:

// path.matchesGlob(path, pattern);

path.matchesGlob('/foo/bar', '/foo/*'); // true
path.matchesGlob('/foo/bar*', 'foo/bird'); // false 

This would enable the globs to be used directly in deciding the value to be assigned to the resolvedHasAllowedExtension variable.

However, the (TypeScript) types indicate that the options.include value(s) can be not only globs but also regular expressions, which cannot be used to append a file extension to the cleanUrl value that is used as the require.resolve() request argument, which is the approach that has been implemented in this PR:

resolved = require.resolve(cleanUrl + allowedExtension, {
    paths: [prefix + scss]
})

Tests

This PR adds tests which simulate the conditions via a setup and teardown task which creates then deletes a mock package directory in the node_modules purely for the amount of time required for the Rollup task to run its build task (to create the compiled files upon which to perform comparisons).

I have called this mock package import-resolution-test, which feels like it should safely avoid clashing with the names of any actual node modules that are present (or are ever likely to become present) in the node_modules directory.

I am not sure if there is a convention for naming such mock packages to avoid any naming clashes (e.g. a leading underscore: _import-resolution-test?).

Release version

Semver versioning recommends that, if approved, these changes, which are essentially a bug fix, should be a patch release:

PATCH version when you make backward compatible bug fixes