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

Webpack5/Vite: Fix sourcemaps #27171

Merged
merged 25 commits into from
May 24, 2024
Merged

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented May 16, 2024

Closes #26954
Closes #26653
Closes ni/nimble#2066

What I did

Fixes sourcemaps for the Webpack5 builder.

Several loaders in the Webpack5 loader chain were not processing source maps properly, resulting in either incorrect or incomplete mapping.

Second, I have configured unplugin for each builder separately so that sourcemaps can be handled individually.

Checklist for Contributors

Testing

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

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

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. Create a Nextjs sandbox
  2. Open Storybook in your browser
  3. Throw an error in the onclick handler within the button story. Click the button and see whether it maps to the correct line (just line mapping works because the default sourcemap option in Webpack5 is set to cheap-module-source-map (A SourceMap without column-mappings that simplifies loader Source Maps to a single mapping per line)
  4. Run Next.js in babel mode by creating a .babelrc file with the following content:
{
  "presets": ["next/babel"],
  "plugins": []
}
  1. Repeat step 3

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 pull request has been released as version 0.0.0-pr-27171-sha-a9f1e1db. Try it out in a new sandbox by running npx storybook@0.0.0-pr-27171-sha-a9f1e1db sandbox or in an existing project with npx storybook@0.0.0-pr-27171-sha-a9f1e1db upgrade.

More information
Published version 0.0.0-pr-27171-sha-a9f1e1db
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/fix-webpack5-sourcemaps
Commit a9f1e1db
Datetime Fri May 24 10:37:30 UTC 2024 (1716547050)
Workflow run 9222633276

To request a new release of this pull request, mention the @storybookjs/core team.

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

@valentinpalkovic valentinpalkovic marked this pull request as draft May 16, 2024 13:53
@valentinpalkovic valentinpalkovic self-assigned this May 16, 2024
@valentinpalkovic valentinpalkovic changed the title Valentin/fix webpack5 sourcemaps Webpack5: Fixes sourcemaps May 16, 2024
Copy link

nx-cloud bot commented May 16, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a9f1e1d. 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 force-pushed the valentin/fix-webpack5-sourcemaps branch from 0f74c00 to 17a5637 Compare May 16, 2024 14:04
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-webpack5-sourcemaps branch from 17a5637 to 19fb74d Compare May 16, 2024 14:09
@valentinpalkovic valentinpalkovic marked this pull request as ready for review May 21, 2024 07:38
@valentinpalkovic valentinpalkovic changed the title Webpack5: Fixes sourcemaps Builder: Fixes sourcemaps for Webpack5 and Vite builder May 22, 2024
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not sure about potential downsides/side effects. I'd say we should properly test with a canary as we don't have any tests in place regarding sourcemaps (I don't even know how we'd be testing this in an automated way)

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Is there some way to do a single automated test of the toolchain in both Webpack and Vite, e.g.

import { Button } from './Button';
/** use jsdoc to to exercise the csf enhancement */
export default { component: Button };
/** again */
export const Basic = {}

So we can (1) verify that it's working properly, and more importantly (2) verify that it doesn't regress later

scripts/tasks/sandbox-parts.ts Outdated Show resolved Hide resolved
@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented May 23, 2024

@shilman I had the idea to use a source map visualization like this one: https://github.com/evanw/source-map-visualization?tab=readme-ov-file. Instead, it analyzes a story and shows the correct source map mapping.

I tried it out for an hour, but I would need more time to get it working somehow. If it would work, we can just easily snapshot things :)

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-webpack5-sourcemaps branch from daf32a6 to 00619f9 Compare May 24, 2024 08:09
Vite/Rollup only needs the "mappings" field for sourecemaps and ignores sourcesContentarrays. Therefore, there is no need to generate "sourcesContent" by setting the "includeContent" flag to true for magicString sourcemaps generation
@valentinpalkovic valentinpalkovic merged commit 8ae46ab into next May 24, 2024
60 of 62 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/fix-webpack5-sourcemaps branch May 24, 2024 12:53
@github-actions github-actions bot mentioned this pull request May 24, 2024
25 tasks
@shilman shilman changed the title Builder: Fixes sourcemaps for Webpack5 and Vite builder Webpack5/: Fixes sourcemaps for Webpack5 and Vite builder May 27, 2024
@shilman shilman changed the title Webpack5/: Fixes sourcemaps for Webpack5 and Vite builder Webpack5/Vite: Fix sourcemaps May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants