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

Add support for configuring Shared Sass Resources for using global mixins, variables, etc. #300

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mikeb-meq
Copy link

Summary/ Motivation (TLDR;)

jest-preview currently supports testing JS that imports SASS, but if the SASS files in the main project use variables, mixins, etc, defined in a shared file, the test will not recognize them. This is addressed on the application side with plugins like e.g. sass-resources-loader for webpack. This feature adds similar support to jest-preview.

Related issues

Features

  • Adds a sharedSassResources option to the API for jestPreviewConfigure. This property takes an array of strings with paths to the SASS files that define shared resources (e.g. variables). The paths are relative to the root of the project being tested. The implementation uses sass.compileString to prepend a "use" statement to all imported SASS, referencing a concatenated copy of the shared resources from the jest-preview cache folder. If the project's version of SASS does not support this method, a warning will be logged and the SASS compilation step will fall back to supported methods.

@netlify
Copy link

netlify bot commented Jun 27, 2023

Deploy Preview for jest-preview-library canceled.

Name Link
🔨 Latest commit bb2dd4a
🔍 Latest deploy log https://app.netlify.com/sites/jest-preview-library/deploys/64ce2846fff90a0008635ac2

@netlify
Copy link

netlify bot commented Jun 27, 2023

Deploy Preview for jest-preview-chinese canceled.

Name Link
🔨 Latest commit bb2dd4a
🔍 Latest deploy log https://app.netlify.com/sites/jest-preview-chinese/deploys/64ce28460641c50007b8d8ac

src/transform.ts Outdated
const buildOptions: BuildCssOptions = { prependUseShared: false};

if (fs.existsSync(sharedSassResourcesPath)) {
buildOptions.prependUseShared = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mikeb-meq I think we can move the logic to create sassPrefixedWithSharedResources to be here, the reasons should be:

  • For consistency with sassLoadPathsConfig. We might update the buildCssResult arguments to `buildCssResult(sass: any, sassLoadPathsConfig: string[], sassPrefixedWithSharedResources: string, filename: string)
  • Can remove BuildCssOptions

src/transform.ts Outdated
return tildeImporter(url);
},
if (options.prependUseShared && !sass.compileString) {
console.warn("WARNING: Config specifies sharedSassResources, but your version of Sass doesn't support it - consider updating to v1.45.0 or higher.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should add a semicolon to the end of this line


## sharedSassResources: string[]

Default: `undefined`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default value should be [], just for consistency with sassLoadPaths, and no need to check that sharedSassResources is existed. Also, I think we can move this doc right below the sassLoadPaths doc, so we wrap all the sass configs in the same area.

Copy link
Collaborator

@ntt261298 ntt261298 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR 😍

There is still one snapshot that failed. Please help to update.
Screenshot 2023-07-09 at 12 13 24
Screenshot 2023-07-09 at 12 13 33

@ntt261298 ntt261298 self-requested a review August 5, 2023 11:12
@mikeb-meq
Copy link
Author

@ntt261298 I apologize but I have been unable to spend extra time at my computer recently due to some health issues. Thank you for assistance with the CI problems. Let me know if I can clarify anything but would try to get to any further fixes when I can.

@ntt261298
Copy link
Collaborator

@ntt261298 I apologize but I have been unable to spend extra time at my computer recently due to some health issues. Thank you for assistance with the CI problems. Let me know if I can clarify anything but would try to get to any further fixes when I can.

Thanks, @mikeb-meq , just waiting for the final approval from @nvh95 then we can deploy a new version where we have this feature. Thanks for your contribution. I wish you good health.

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.

2 participants