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 sass-resources-loader type functionality for loading SASS files #297

Open
mikeb-meq opened this issue May 17, 2023 · 11 comments
Open

Add sass-resources-loader type functionality for loading SASS files #297

mikeb-meq opened this issue May 17, 2023 · 11 comments
Labels
feature request Request a new feature

Comments

@mikeb-meq
Copy link

Is your feature request related to a problem? Please describe.

I have set up jest-preview in my project and it seems to work well other than loading my styles (with SASS). When I add the following config to my jest.config.js, my test starts encountering "Undefined variable."

module.exports = {
    transform: {
        "^.+\\.(css|scss)$": "jest-preview/transforms/css",

This is happening because I use global SASS variables defined in separate files, and load those using sass-resources-loader with webpack for when the application runs. I found the existing option for globally loading css, but that seems to load the files individually rather than prepending them to all imported scss like sass-resources-loader does.

Describe the solution you'd like

Ideally adding a config option for prepending a set of scss files to all imported scss, like sass-resources-loader supports.

Describe how should jest-preview implements this feature

This could be potentially implemented by reading the contents of the global files and concatenating them with each imported scss using sass.compileString rather than sass.compile, but I don't know how efficient that would be.

Describe alternatives you've considered

I couldn't think of any alternatives that wouldn't require changing application code as a workaround.

I would be happy to try contributing this feature if I could be pointed to the best place to start.

@mikeb-meq mikeb-meq added the feature request Request a new feature label May 17, 2023
@nvh95
Copy link
Owner

nvh95 commented May 18, 2023

Thank you @mikeb-meq. We will look into that.
@ntt261298 Can you help to own this issue? You can guide @mikeb-meq the place to contribute as well. Thanks.

@ntt261298
Copy link
Collaborator

ntt261298 commented May 20, 2023

@mikeb-meq
Thank you for your detailed issue description. Your solution does make sense. I think sassResources might be the suitable name for the config that accepts an array of Sass files to be prepended to all imported Sass files.

To support this case, we can replace sass.compile with sass.compileString. There should be no performance impact.

We appreciate your contribution to the project. Here are the steps you can get started with:

  1. Read the contributing doc to understand the code structure as well as how to submit a PR
  2. Add a new config here
  3. Update the processSass function:
    if (sass.compile) {
  • I think we can refer to the logic from sass-resources-loader
  • Note that jest-preview also supports Sass version < 1.45.0, where sass.compileString doesn't exist. Consider logging a warning to indicate that the config cannot be applied in this case.
  1. To ensure the solution works, build jest-preview locally and use the NPM link to link the local jest-preview with your project.

Thanks.

@mikeb-meq
Copy link
Author

Thanks for building this great looking project and for the quick response! I will update here when I can dedicate some time to this, hopefully in the next few weeks.

@mikeb-meq
Copy link
Author

Hi @ntt261298 - I was able to revisit this this week and have a working implementation for the feature. I needed to branch off the 0.3.1 tag however since the 0.3.2 alpha release was giving me errors trying to run the server:

jest-preview mikeb-meq$ pnpm run server

> jest-preview@0.3.2-alpha.1 server /Users/mikeb-meq/git-public/jest-preview
> node dist/cli/index.js

/Users/mikeb-meq/git-public/jest-preview/dist/cli/index.js:5
var updateNotifier = require('update-notifier');
                     ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/mikeb-meq/git-public/jest-preview/node_modules/.pnpm/update-notifier@6.0.2/node_modules/update-notifier/index.js from /Users/mikeb-meq/git-public/jest-preview/dist/cli/index.js not supported.
Instead change the require of /Users/mikeb-meq/git-public/jest-preview/node_modules/.pnpm/update-notifier@6.0.2/node_modules/update-notifier/index.js in /Users/mikeb-meq/git-public/jest-preview/dist/cli/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/mikeb-meq/git-public/jest-preview/dist/cli/index.js:5:22) {
  code: 'ERR_REQUIRE_ESM'
}

Do you have any suggestions on how to resolve that? I can still create a PR with my commits cherry picked on a branch from 0.3.2 alpha, but I have not been able to test the code in that state.

@ntt261298
Copy link
Collaborator

Hi @mikeb-meq

jest-preview has an issue with update-notifier@6, which is resolved by this PR. You can update src/cli/index.ts to temporarily resolve this issue from your local machine:

import { program } from 'commander';
import chalk from 'chalk';

program
  .command('config-cra')
  .description('Integrate Jest Preview with CRA.')
  .action(() => {
    import('./configCra');
  });

program
  .command('clear-cache')
  .description('Clear Jest and Jest Preview cache.')
  .action(() => {
    import('./clearCache');
  });

program.description('Start Jest Preview server.').action(() => {
  import('./server/previewServer');
});

program.parse(process.argv);

import('update-notifier').then(({ default: updateNotifier }) => {
  // Checks for available update and notify user
  const notifier = updateNotifier({
      // Built output is at /cli so the relative path is ../package.json
    pkg: require('../../package.json'),
    updateCheckInterval: 0, // How often to check for updates
    shouldNotifyInNpmScript: true, // Allows notification to be shown when running as an npm script
    distTag: 'latest', // Can be use to notify user about pre-relase version
  });

  notifier.notify({
    defer: true, // Try not to annoy user by showing the notification after the process has exited
    message: [
      `${chalk.blue('{packageName}')} has an update available: ${chalk.gray(
        '{currentVersion}',
      )}${chalk.green('{latestVersion}')}`,
      `Please run ${chalk.cyan('`{updateCommand}`')} to update.`,
    ].join('\n'),
  });
  // END checks for available update and notify user
});

Thanks for revisiting jest-preview!

@mikeb-meq
Copy link
Author

Hi @ntt261298 - thank you for your quick reply - applying that patch locally allowed me to run everything from the latest alpha branch.

As far as testing my feature, I was able to confirm it is working with the "demo" app within jest-preview itself (using npx jest App.test), but in following

  1. To ensure the solution works, build jest-preview locally and use the NPM link to link the local jest-preview with your project.

to test it from my own project, I encounter this error when using import { jestPreviewConfigure } from 'jest-preview';

Test suite failed to run

    Cannot find module 'core-js/modules/web.dom.iterable.js' from '../../../../../git-public/jest-preview/dist/index.js'

    Require stack:
      /Users/mikeb-meq/git-public/jest-preview/dist/index.js
      testing/jest-setup/jest-preview-setup.js

Do you have any suggestions for resolving this?

Is there any other testing I should perform before opening a PR back to your repo for the feature?

@ntt261298
Copy link
Collaborator

ntt261298 commented Jun 25, 2023

Hi @mikeb-meq

Looks like I can still link jest-preview locally without seeing the above error.
Can you provide the steps that you followed, the node, and the npm version that you used? I will try to use that information to reproduce your issue on my local machine. Also, It would be great if you can share a sample app that encounters the issue.

After checking that the new update works on your project, you can open a PR to jest-preview repo. Make sure to run the test pnpm run test and update the test snapshot if needed.

Thanks.

@mikeb-meq
Copy link
Author

Hi @ntt261298 - thanks for the reply. I'm not sure what was causing the above error for me, but my project has a dependency on "core-js": "2.6.9" and I was able to resolve it by installing that same package to my local repo of jest-preview (only for testing). I verified that my project does run as expected now using preview.debug() inside a test that imports SASS containing shared variables.

The results of pnpm run test from jest-preview are:

 PASS  demo/__tests__/App.test.tsx (14.813 s)
  App
    ✓ should work as expected (300 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        15.696 s
Ran all test suites matching /App/i.

and it loads the demo app page with my new text entry:
image

I have opened a PR for the feature. Should I also update the documentation for jestPreviewConfigure?

@ntt261298
Copy link
Collaborator

Hi @mikeb-meq

Thanks for your PR. Let's help to update the documentation for jestPreviewConfigure.

@mikeb-meq
Copy link
Author

@ntt261298 Okay, I pushed a commit to update that documentation.

@ntt261298
Copy link
Collaborator

@mikeb-meq I've added some comments to the PR, please help to take a look.

Thank you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request a new feature
Projects
None yet
Development

No branches or pull requests

3 participants