Skip to content

Conversation

@amannn
Copy link
Contributor

@amannn amannn commented Jun 8, 2021

Since division with / is being deprecated I stumbled upon two issues while moving to sass:math with sass-resources-loader.

This PR addresses both of them:

  • Hoisted imports should be de-duplicated so SASS doesn't throw. I'd argue that this is necessary, since an individual style sheet can't know which imports are used in a resource and also shouldn't rely on them.
  • Imports to built-ins from SASS shouldn't be rewritten.

I've tested the changes from this PR locally in my project and everything worked fine then.

Thank you for maintaining sass-resources-loader!


This change is Reviewable


exports[`sass-resources-loader imports should not rewrite the path for built-ins with @use 1`] = `
"// Should be de-duplicated
@use 'sass:math';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this was ../sass:math

@amannn
Copy link
Contributor Author

amannn commented Jun 16, 2021

@justin808 Do you think you'd have time for a brief review here?

@justin808
Copy link
Member

Hi @amannn, thanks for reaching out, and thanks for your contribution!

I took a glance, and it looks good.

How confident are you that:

  1. Your change won't cause any regressions.
  2. You added adequate tests.
  3. You added adequate documentation.
  4. You added a changelog entry. What should be the next version number?

@Tomburgs, any chance that you can take a quick look

@amannn amannn closed this Jun 16, 2021
@CyberCookie
Copy link

Please merge!
There is no way to @use "sass:math" in sass files together with this lib since the lib adds resources to the very top of sass files and placing @use rule manually produces rules ordering error
sass use
"A stylesheet’s @use rules must come before any rules..."

@amannn amannn reopened this Jun 16, 2021
@amannn
Copy link
Contributor Author

amannn commented Jun 16, 2021

Hi @justin808,

thanks for having a look!

I've added some new tests and the previous tests seem to still work, so I hope the change is ok. An in-depth look from you would of course be really helpful. There are no necessary docs changes, as this is only a compatibility fix. I've added a changelog entry and bumped the version number as requested.

Sorry, I closed the PR by accident before but reopened it now.

@justin808 justin808 merged commit 230c4a1 into shakacode:master Jun 17, 2021
@justin808
Copy link
Member

@CyberCookie @amannn 2.2.2 pushed to npm!

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