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

Next.js: Avoid interfering with the svgr loader #27198

Merged
merged 1 commit into from
May 21, 2024

Conversation

seanparmelee
Copy link
Contributor

@seanparmelee seanparmelee commented May 19, 2024

Closes #27195

What I did

This is a followup to #27093 as it causes issues for anyone who has followed the example for adding SVGR support to Storybook. Instead of adding a test to the raw loader, this PR uses exclude so the rule doesn't apply to __barrel_optimize__ requests.

Checklist for Contributors

Testing

As I mentioned in #27093, if there's a good way to cover this scenario as part of an integration and/or e2e test, please let me know.

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

There is a reproduction in #26991 (https://github.com/11bit/next-storybook-import-raw-bag) that could be used to confirm this story loads with these changes.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@seanparmelee
Copy link
Contributor Author

@valentinpalkovic as part of this, should I update the svgr example so that it checks for all image rules rather than the first one?

Copy link

nx-cloud bot commented May 19, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8a84014. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic valentinpalkovic added bug patch:yes Bugfix & documentation PR that need to be picked to main branch ci:normal labels May 19, 2024
@valentinpalkovic valentinpalkovic self-assigned this May 19, 2024
@valentinpalkovic
Copy link
Contributor

Thank you so much for putting this together that fast!

If you want to adjust the docs, it would be appreciated. Otherwise, I'll take care of it together with @jonniebigodes.

@seanparmelee
Copy link
Contributor Author

I'll leave it for you and @jonniebigodes

@seanparmelee
Copy link
Contributor Author

@valentinpalkovic FWIW, when I copy the Typescript version of the example into our project, we have a couple TS errors. We have strict enabled, which I suspect is the reason we see these:

The line that has

const imageRule = config.module.rules.find((rule) => rule?.['test']?.test('.svg'));

Has the following error

Element implicitly has an 'any' type because expression of type '"test"' can't be used to index type 'false | "" | 0 | RuleSetRule | "..."'.
  Property 'test' does not exist on type 'false | "" | 0 | RuleSetRule | "..."'.ts(7053)

And the line that has

imageRule['exclude'] = /\.svg$/;

Has the following error:

Element implicitly has an 'any' type because expression of type '"exclude"' can't be used to index type 'RuleSetRule | "..."'.
  Property 'exclude' does not exist on type 'RuleSetRule | "..."'.ts(7053)

Replacing that code with your suggestion, this is one way to make (strict) TS happy:

config.module.rules.forEach((rule) => {
      if (
        typeof rule === 'object' &&
        rule?.['test'] instanceof RegExp &&
        rule['test'].test('.svg') &&
        typeof rule.issuer === 'object' &&
        'not' in rule.issuer &&
        rule.issuer.not instanceof RegExp &&
        rule.issuer.not?.test('.css')
      ) {
        rule['exclude'] = /\.svg$/;
      }

      return rule;
    });

🥴 There's probably a better/more elegant way to do this, but I also wonder if the logic should be more lenient with the rule.issuer checks.

@valentinpalkovic
Copy link
Contributor

I agree. Being fully TypeScript compliant is sometimes really horrible. Let me know if you find a more elegant solution to this issue.

@valentinpalkovic valentinpalkovic merged commit 84fa690 into storybookjs:next May 21, 2024
56 of 58 checks passed
@github-actions github-actions bot mentioned this pull request May 21, 2024
25 tasks
storybook-bot pushed a commit that referenced this pull request May 21, 2024
Next.js: Avoid interfering with the svgr loader

(cherry picked from commit 84fa690)
@github-actions github-actions bot mentioned this pull request May 21, 2024
8 tasks
storybook-bot pushed a commit that referenced this pull request May 21, 2024
Next.js: Avoid interfering with the svgr loader

(cherry picked from commit 84fa690)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: After upgrade to v8.1.1 svg loader doesn't work for nextjs
2 participants