-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
feat: extend conditionNames #1092
feat: extend conditionNames #1092
Conversation
This allows consumers to set their own considitionNames and have sass pick this up. Examples: Supporting a single entrypoint but targeting multiple themes ```scss @use 'my-design-tokens'; .class { color: my-design-tokens.$color-1 } ``` package.json ```json { "name": "my-design-tokens", "exports": { ".": { "sass": { "theme1": "./path-to-theme1.scss", "theme2": "./path-to-theme2.scss" } } } } ``` Webpack config ``` module.exports = { resolve: { conditionNames: [someFlag ? "theme1" : "theme2", "..."] } } ```
|
@alexander-akait @snitin315 Looking for comments on this. This seems like a reasonable extension to the sass loader. I was concerned that this may cause issues if it inherits webpack's conditionNames, as it would start resolving |
@ersachin3112 Apologies for tagging you here on my first comment. Was not sure who the maintainers were and saw many commits from you. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with it, but let's add tests:
- Custom
conditionName
(as in your example) - Case where you can show that we avoid using
import
/module
/etc
Codecov ReportBase: 94.10% // Head: 94.10% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #1092 +/- ##
=======================================
Coverage 94.10% 94.10%
=======================================
Files 5 5
Lines 373 373
Branches 137 137
=======================================
Hits 351 351
Misses 19 19
Partials 3 3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
const pathToSassPackageWithExportsFieldsAndCustomConditionReplacer = () => { | ||
if (context.packageExportsCustomConditionTestVariant === 1) { | ||
return path.resolve( | ||
testFolder, | ||
"node_modules/package-with-exports-and-custom-condition/style-1.scss" | ||
); | ||
} | ||
|
||
if (context.packageExportsCustomConditionTestVariant === 2) { | ||
return path.resolve( | ||
testFolder, | ||
"node_modules/package-with-exports-and-custom-condition/style-2.scss" | ||
); | ||
} | ||
|
||
console.warn( | ||
"Expedted to receive .packageExportsCustomConditionTestVariant to properly resolve stylesheet in sass only compilation. " | ||
); | ||
return ""; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There wasn't a great pattern to write the type of tests that made sense here so I had to introduce some new constructs (such as this context
param) to tell getCodeFromSass
how to properly resolve the file that the tests need to compare against (based on the custom condition).
@alexander-akait I've updated the tests per your request. I did not explicitly add a test to check that there is no conflict with the Let me know if you would like an explicit test but that felt like an overkill in an already expensive testsuite 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with it, there is only one exception, if you will have for example theme1
and it will point to js file (or json) you will get an error, so I recommend to use sass:theme1
to avoid such problems (mostly webpack will not have the such problem due restriction
, but Node.js or other runtimes potential can have)
Thanks @alexander-akait ! 💪🏽 |
This PR contains a:
Motivation / Use-Case
This allows consumers to set their own considitionNames and have sass pick this up.
Examples:
Supporting a single entrypoint but targeting multiple themes
package.json
Webpack config
Breaking Changes
Additional Info