nvh95 / jest-preview

Debug your Jest tests. Effortlessly.🛠🖼
https://www.jest-preview.com
MIT License
2.34k stars 76 forks source link

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

Open mikeb-meq opened 1 year ago

mikeb-meq commented 1 year ago

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.

nvh95 commented 1 year ago

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 commented 1 year ago

@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
  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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Hi @mikeb-meq

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

mikeb-meq commented 1 year ago

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

ntt261298 commented 1 year ago

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

Thank you again.