Migrate from @import to @use at-rules

[tannerdolby]

Migrates all @import at-rules to @use in preparation for dart-sass deprecating @import at-rules.

Note: There are still 4 @import at-rules left in two files (README.md and test/index.css) in the project. But we can discuss converting these or leaving them during review. We started out at 41 @import results in 5 files and took it down to 4 results in 2 files which is exciting!

Types of changes

• Bug fix (a non-breaking change which fixes an issue)
• New feature (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves #123

Replaces all existing @import at-rules with @use because @import will eventually be deprecated in dart-sass in the coming years.

Link(s)

Screenshot(s)

Checklist:

• I have thoroughly read the CONTRIBUTING guidelines.
• I understand my pull request will be thoroughly reviewed at high detail.
• I understand the work in my pull request will only be available in the next version release of Checka11y.css and not in the current version release.
• I confirm the work in this pull request is valid according to my findings and is not something for anything personal.
• I have updated the README and/or features.md where and if applicable (still put an x if you have considered this but thought there was nothing to add or modify).
• I have added myself to the contributors section in package.json (still put an x if you have considered this but decided not to add yourself).
• I have checked I have not committed any accidental files.
• I have tested all the main modern browsers (I.e. Chrome, Firefox, Edge, Safari - please leave this unchecked if there were any browsers listed you could not test and list them in the help section with details why you couldn't test that browser)
• I have run the automated tests and added new ones to cover new code.
• All new and existing a11y checks still work correctly (compare your local test/index.html to the test/index.html in the master branch).

[jackdomleo7]

Looks really good @tannerdolby, thank you!
Just 1 comment, interested to hear your thoughts on it before approving.

Also, seeing all these ../../ makes me wonder if we can add aliases to SCSS where ~ is an alias for src?

[jackdomleo7]

Will this change cause a breaking change? I.e. will I need to release a new major version of this package?

[jackdomleo7]

Also, would the usage docs need updating?

[tannerdolby]

Ok great @jackdomleo7, your welcome!

I'm not entirely sure if we can add aliases to SCSS, although the constant ../../ usage can get cumbersome, I think for now that is the only way. I will do some tinkering/research and see if anyone else is aliasing part of the URL when using @use.

[tannerdolby]

I don't think this will cause a breaking change from reviewing sass - breaking changes, but just to be safe it might be best to release a new major version for the package so if anyone wants to move forward with the @use at-rule usage they can.

Otherwise its fine for users to stay at the prev version (with @import) and they can use it until import is eventually deprecated.

[tannerdolby]

Yeah, the usage docs would indeed need to be updated for L124 and L127 in the README.md.

[tannerdolby]

The @import rules replaced in the README.md from the commit above introduce @use "<url>" as * where the as * might not be necessary when loading the entire file for compilation but if anything is going to referenced in the file which its loaded (with @use) by users (referencing utilities like mixins or variables etc) then having no namespace attached with @use "<url>" as * will be the safest bet to ensure everything works as it did before.

[jackdomleo7]

I agree, let's leave the aliasing for now as it's not important for this work.

This is a good PR, for my own comfort, I'm going to do some investigation so I fully understand what's going on here - so this is going to delay the PR for a short while. 🙂 But I really appreciate this being highlighted and worked on!

[jackdomleo7]

I've looked into this a little further and these are my findings:

• As expected anyway, any project using node-sass rather than sass will not be able to use this implementation (which makes me think, should we add sass as a peer dependency?), whereas anyone using node-sass with v1 wouldn't experience any issues
• Projects already using sass will not encounter a breaking change. However, if a project is overriding any of the !default SCSS variables, then they will encounter a breaking change whereby there overriding variables will not be picked up. The Sass compilation will not error.

I'm happy with everything in this PR, especially now that I understand it all. I have decided I am going to deploy this as a new major as we discussed earlier above. With this in mind, I will deploy a new minor version first (before completing this PR) so that people using v1 can make use of new features awaiting to be deployed without having to upgrade to v2 to utilise them. I will approve this PR once it is ready to be merged. Would you be happy to maintain this PR and keep it updated with master in the meantime @tannerdolby?

Thanks once again for this PR!

[tannerdolby]

Your welcome! 🚀

I'm not super familiar with peer dependencies, but I do think that adding sass as a peer dependency wouldn't be a bad idea so its not automatically installed. And like you said anyone using node-sass with v1 won't experience any difference. Ok good to know, so only if a project is overriding any of default Sass variables will it cause a breaking change where the overridden variables are not usable. Interesting there isn't a compilation error.

Ok great! Yeah, I agree. Deploying a new minor version before this is merged will be a good idea so anyone using v1 can make use of new features waiting to be deployed without having to jump all the way to v2. Yes I'm happy to maintain this PR and keep it up-to-date with master until were ready to move forward.

[jackdomleo7]

Hi @tannerdolby. I have released the new minor. Whenever you are ready, could you update this branch with master, then I'd be happy to approve so we can merge.

In the meantime, I'm going to be working on v2 milestone issues

[tannerdolby]

Hi @jackdomleo7! Ok sounds good. I will merge with master shortly and get things up to date here.

Got it, I will have a look over at the milestone issues also. Thanks again for all your review throughout this PR :)

[tannerdolby]

I merged with upstream master but there were some merge conflicts, I fixed them locally and committed but it looks like the Pipeline build failed. I will see what I can do to resolve this.

For some context,

Exiting because `git status` is not empty:

   M checka11y-errors.css
   M checka11y-warnings.css
   M checka11y.css

[jackdomleo7]

It failed on the "Is git clean" step. Essentially this is a step in the pipeline run after npm run build. If it fails, it means the generated CSS files don't match up.
The fix is to run npm run build locally, then commit the changes 🙂

Thank you!

[jackdomleo7]

Thanks a lot for this @tannerdolby

[tannerdolby]

Your welcome @jackdomleo7!