Skip to content

Conversation

bryanjtc
Copy link
Contributor

The repository for css-modules-loader-core hasn't been updated in years, and it has vulnerabilities.
I found a replacement that is a fork from css-modules-loader-core and has been rewrited in typescript.
When I ran yarn install I had trouble with node-sass, so I updated it. You can undo that change.

@bryanjtc bryanjtc force-pushed the update-node-sass-&-change-css-modules-loader-core branch from 5a2e4de to 0edbf38 Compare May 14, 2022 02:49
@bryanjtc bryanjtc force-pushed the update-node-sass-&-change-css-modules-loader-core branch from 0edbf38 to 61db016 Compare May 14, 2022 02:58
@skovy
Copy link
Owner

skovy commented Jun 3, 2022

hey, thanks for the contribution!

it doesn't look like there is much usage of this package (based on npm downloads / GitHub activity). therefore, I don't think I'm comfortable adding this package as a dependency. if the current package has vulnerabilities that's not good, but do you know what they are? it's possible it depends on packages that would have a vulnerability in the browser but wouldn't be a true threat running in a script locally. not saying that's the case here, but we should confirm the vulnerability is a true threat to users of this package before we swap to a random fork (which could pose a bigger threat)

as for node-sass, I think upgrading the version seems reasonable but since this package still supports v4 I think it's probably best we continue using the lowest common denominator (lowest supported version) for development to ensure it's still supported by the package

@sfreedgood
Copy link

@skovy Context Regarding node-sass
node-sass ^7.0.0 supports Node 12, 14, 16, 17
node-sass ^6.0.0 supports Node 12, 14, 15, 16
node-sass ^5.0.0 supports Node 10, 12, 14, 15 and introduces new node-gyp version that supports building with Python 3
node-sass ^4.14.0 supports Node 0.10, 0.12, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14
node-sass ^4.12.0 (current) supports Node 0.10, 0.12, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12

FWIW [email protected] introduces support for Node v16, which is the minimum version supported by ARM (and by extension any Apple machines containing M1 chips).

@skovy
Copy link
Owner

skovy commented Jun 15, 2022

@sfreedgood thanks for the context.

This package includes a specific node-sass version in devDependencies which should only be installed for development. There is an (optional) peer dependency for node-sass so you can use any version greater than this version in your projects.

Am I missing something? There is no harm in keeping this package on version 4 and supporting more node-sass versions for people that may want to use this on an older version. If there are features or reasons to upgrade (eg: dropping EOL node versions) I think that's reasonable, but that's not what this PR is aiming to do.

I'm open to contributions here but I'd like to see a PR focused on dropping EOL node versions and maybe upgrading to the minimum node-sass version to reflect that, along with the context on why it's necessary.

I also don't want to switch to a random unused fork of a project which introduces larger security risks. For these reasons, I'm closing this PR.

@skovy skovy closed this Jun 15, 2022
@sfreedgood
Copy link

For anyone having issues with the node-sass version in their peer-dependencies, the solution I ended up using was:
npm v8 supports overrides, which you can add to your package.json:

"overrides": {
	"typed-scss-modules": {
		"node-sass": "7.0.1"
	}
}

cc @bryanjtc if it helps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants