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.

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

Merged
merged 2 commits into from Oct 6, 2022

Conversation

GeorgeTaveras1231
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

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

@use 'my-design-tokens';

.class {
  color: my-design-tokens.$color-1
}

package.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", "..."]
  }
}

Breaking Changes

Additional Info

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", "..."]
  }
}
```
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: GeorgeTaveras1231 / name: George (eaef60d)

@GeorgeTaveras1231
Copy link
Contributor Author

GeorgeTaveras1231 commented Oct 5, 2022

@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 import, or module conditions. However, I tested this locally and it seems like it still ignores import and module exports. My assumption is this is due to webpack's internal handling of conditions, where it seems its able to detect that the reference is made from sass so it doesn't set the module or import conditions (I read this here). This makes me feel as though this change is completely safe. And adds some more value to the sass-loader.

@GeorgeTaveras1231
Copy link
Contributor Author

GeorgeTaveras1231 commented Oct 6, 2022

@ersachin3112 Apologies for tagging you here on my first comment. Was not sure who the maintainers were and saw many commits from you. 😅

alexander-akait
alexander-akait previously approved these changes Oct 6, 2022
Copy link
Member

@alexander-akait alexander-akait left a 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:

  1. Custom conditionName (as in your example)
  2. Case where you can show that we avoid using import/module/etc

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Base: 94.10% // Head: 94.10% // No change to project coverage 👍

Coverage data is based on head (ec88eef) compared to base (3a34fef).
Patch has no changes to coverable lines.

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           
Impacted Files Coverage Δ
src/utils.js 92.76% <ø> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +59 to +78
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 "";
};
Copy link
Contributor Author

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).

@GeorgeTaveras1231
Copy link
Contributor Author

@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 import and module conditions -- instead, I moved the import condition higher up in the package.json that was being used (here and here). By doing this, we are stress testing this potential issue, because the import condition should take precedence (given they come first in the list of exports). Therefore, if there was a conflict, the current tests would fail because they would be unable to resolve the desired scss files.

Let me know if you would like an explicit test but that felt like an overkill in an already expensive testsuite 😅

Copy link
Member

@alexander-akait alexander-akait left a 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)

@alexander-akait alexander-akait merged commit 6e02c64 into webpack-contrib:master Oct 6, 2022
@alexander-akait
Copy link
Member

@GeorgeTaveras1231
Copy link
Contributor Author

Thanks @alexander-akait ! 💪🏽

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.

None yet

2 participants