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.

Sign up for GitHub

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

Jump to bottom

Fix Sass + CSS Modules #3186

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Fix Sass + CSS Modules #3186

merged 1 commit into from
Apr 19, 2021

Conversation

drwpow
Copy link
Collaborator

@drwpow drwpow commented Apr 19, 2021 •
edited

Changes

Fixes issue with *.module.scss and *.module.sass files as reported in #2998.

Testing

Tests added; there weren’t any tests before. Tests should pass.

Docs

@drwpow drwpow requested a review from a team as a code owner April 19, 2021 19:40
@vercel
Copy link

vercel bot commented Apr 19, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/HaBen3wWHZbfPQoTcrmBzcDPZ4Hj
✅ Preview: https://snowpack-git-sass-css-modules-pikapkg.vercel.app

@drwpow drwpow mentioned this pull request Apr 19, 2021
Open
4 tasks
@natemoo-re natemoo-re linked an issue Apr 19, 2021 that may be closed by this pull request
[BUG] [3.1.2] CSS modules support is broken when optimize.bundle is true #2998
Open
4 tasks
natemoo-re
natemoo-re reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one clarification question.

snowpack/src/build/import-css.ts
return (
url.endsWith('.module.css') || url.endsWith('.module.scss') || url.endsWith('.module.sass')
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checking, these are the only suffixes we need? Do we support user-provided preprocessors (.module.less, etc)?

Copy link
Collaborator Author

@drwpow drwpow Apr 19, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be right—this may change behavior for .module.less, but that support probably has other untested problems (and there’s never been an official LESS plugin either). So I wouldn’t say we’re “dropping” support there.

.module.* in general is a Snowpack internal behavior we never really exposed to external plugins, so there’s never really been a user-provided interface for it, no.

@drwpow drwpow merged commit 758bdb2 into main Apr 19, 2021
@drwpow drwpow deleted the sass-css-modules branch April 19, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natemoo-re natemoo-re natemoo-re left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] [3.1.2] CSS modules support is broken when optimize.bundle is true
2 participants
@drwpow @natemoo-re