jest-preview icon indicating copy to clipboard operation
jest-preview copied to clipboard

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

Open mikeb-meq opened this issue 1 year ago • 11 comments

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 avatar May 17 '23 15:05 mikeb-meq

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.

nvh95 avatar May 18 '23 02:05 nvh95

@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: https://github.com/nvh95/jest-preview/blob/880630f4165a55a7e2cced981b1275e32416e5ee/src/transform.ts#L415
  • 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.

ntt261298 avatar May 20 '23 08:05 ntt261298

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 avatar May 22 '23 13:05 mikeb-meq

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

> [email protected] 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/[email protected]/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/[email protected]/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.

mikeb-meq avatar Jun 16 '23 16:06 mikeb-meq

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!

ntt261298 avatar Jun 17 '23 12:06 ntt261298

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?

mikeb-meq avatar Jun 22 '23 16:06 mikeb-meq

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.

ntt261298 avatar Jun 25 '23 14:06 ntt261298

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?

mikeb-meq avatar Jun 27 '23 16:06 mikeb-meq

Hi @mikeb-meq

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

ntt261298 avatar Jun 29 '23 02:06 ntt261298

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

mikeb-meq avatar Jul 07 '23 21:07 mikeb-meq

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

Thank you again.

ntt261298 avatar Jul 09 '23 05:07 ntt261298