rollup-plugin-scss
rollup-plugin-scss copied to clipboard
Handle node module duplicate filename import resolution
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:
-
cleanUrl
:package-name/main
-
resolved
:/{path}/app-name/node_modules/package-name/main.js
.
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