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

feat: add sourcemapIgnoreList configuration option #12174

Merged
merged 8 commits into from
Mar 1, 2023

Conversation

danielroe
Copy link
Contributor

@danielroe danielroe commented Feb 23, 2023

Description

This PR adds support for adding hints to ignore library sources in the sourcemaps served up by the dev server. (Rollup support for sourcemaps in build was added in rollup/rollup#4848 and is configurable through build.rollupOptions.output.sourcemapIgnoreList - see nuxt implementation.)

It's largely a copy of @bmeurer's code from that PR to Vite.

In addition, it defaults to ignoring sources that are within node_modules, though this can of course be overridden by users/frameworks using vite.

I've marked as a draft so we can discuss further the API of the function and what else we might like to do in this particular function - in particular, it might be an opportunity to elevate the changes in #12079 to this point, applying them either here (or just after this function is called).

Note: this PR came out of a conversation with @bmeurer so would appreciate a Co-authored-by crediting him (or he may wish to open a new PR with a slightly different take on this one).

Before After
CleanShot 2023-02-23 at 22 50 06@2x CleanShot 2023-02-23 at 22 48 59@2x

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bmeurer
Copy link
Contributor

bmeurer commented Feb 24, 2023

I've marked as a draft so we can discuss further the API of the function and what else we might like to do in this particular function - in particular, it might be an opportunity to elevate the changes in #12079 to this point, applying them either here (or just after this function is called).

I updated #12079 to use this choke-point as well, which seems to catch all the cases now (still puzzling to me why sometimes .jsx doesn't take the transformWithEsbuild route though).

Co-authored-by: patak <matias.capeletto@gmail.com>
@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Feb 24, 2023
@patak-dev
Copy link
Member

Labeled as P3 so we get to it in the next team meeting (in 10 days) before releasing 4.2. We may get to it async sooner though.

@bluwy
Copy link
Member

bluwy commented Feb 26, 2023

Does Nuxt need to pass to server.sourcemapIgnoreList? It would be nice if Vite provided a good default ootb, which the node_modules check seems good enough, and we can remove the option altogether.

@danielroe
Copy link
Contributor Author

Yes, we also likely want to exclude generated virtual files.

@bluwy
Copy link
Member

bluwy commented Feb 26, 2023

Does virtual files in Nuxt use the \0 or virtual: convention? We could perhaps handle that by default too. But if we do need an option still, couldn't we re-use Rollup's output.sourcemapIgnoreList option? Otherwise the same predicate needs to be passed in two places, or are there cases where it needs to differ.

@bmeurer
Copy link
Contributor

bmeurer commented Feb 27, 2023

Does virtual files in Nuxt use the \0 or virtual: convention

What is this convention? Have never heard of it, but this sounds very relevant to DevTools. Do you have links / docs?

@bluwy
Copy link
Member

bluwy commented Feb 27, 2023

They are usually conventions in Rollup and Vite: https://rollupjs.org/plugin-development/#conventions and https://vitejs.dev/guide/api-plugin.html#virtual-modules-convention, where in the resolveId hook, plugins can prepend a \0 (null byte) to tell "I'm handling this file alone" (though it's still up to other Rollup/Vite plugins whether they want to skip transforms if they see a \0).

virtual: is the user-facing identifier convention, and \0 is the internal resolved convention. I think sourcemap sources uses the latter (?)

@bmeurer
Copy link
Contributor

bmeurer commented Feb 27, 2023

They are usually conventions in Rollup and Vite: https://rollupjs.org/plugin-development/#conventions and https://vitejs.dev/guide/api-plugin.html#virtual-modules-convention, where in the resolveId hook, plugins can prepend a \0 (null byte) to tell "I'm handling this file alone" (though it's still up to other Rollup/Vite plugins whether they want to skip transforms if they see a \0).

virtual: is the user-facing identifier convention, and \0 is the internal resolved convention. I think sourcemap sources uses the latter (?)

Thanks for the pointers. That explains the @id/<foo> that we see in DevTools.

@danielroe
Copy link
Contributor Author

We also have other files in .nuxt we would want to exclude as well (.nuxt is our buildDir and also includes our virtual file system). Here's the current proposed implementation for rollup settings: nuxt/nuxt#19243.

build.rollupOptions.output.sourcemapExcludeSources doesn't sound like the right option for configuring the dev server. But as long as we have a way to configure this, I'm happy.

In any case, it seems like a good default. But one implementation question: IIRC, it's possible to specify more than one output in rollup; do you know if that is supported by vite (or in use by other frameworks)?

@danielroe danielroe marked this pull request as ready for review February 27, 2023 09:48
docs/config/server-options.md Outdated Show resolved Hide resolved
packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

If we would like to share a single option for build and the dev server, I think we should have it be a shared option (on the root of the config object, but then we have to read both the shared one and the rollup one). It makes sense to me to have a separate option for the server and build, it could be more flexible and these options are going to be used normally by frameworks more than final users.

@bluwy
Copy link
Member

bluwy commented Feb 27, 2023

Hmm yeah looks like you can specify multiple outputs, I guess we could go with a new option then. We do sometimes refer to build related config in dev so I thought that could work too. To keep things simple for now, I'd be fine going with this PR's approach, especially given that it's setting a special field in sourcemaps now.

@patak-dev
Copy link
Member

We discussed with Evan about this PR today, we can move forward after applying the sourcemapIgnoreList: false addition 👍🏼

- **Type:** `false | (sourcePath: string, sourcemapPath: string) => boolean`
- **Default:** `(sourcePath) => sourcePath.includes('node_modules')`

Whether or not to ignore source files in the server sourcemap, used to populate the [`x_google_ignoreList` source map extension](https://developer.chrome.com/blog/devtools-better-angular-debugging/#the-x_google_ignorelist-source-map-extension).
Copy link
Member

Choose a reason for hiding this comment

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

@bmeurer not blocking for the PR, but it would be nice to be able to link to a more focused blog post about x_google_ignoreList. If your team ends up having a better resource in the future, would you do a PR to rewire these references?

Copy link
Contributor

@jecfish jecfish Mar 1, 2023

Choose a reason for hiding this comment

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

+1 to this, I will draft something up and send another PR to update it. Thanks for calling that out!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jecfish, sounds great!

@patak-dev patak-dev changed the title feat(vite): add sourcemapIgnoreList configuration option feat: add sourcemapIgnoreList configuration option Mar 1, 2023
@patak-dev patak-dev merged commit f875580 into vitejs:main Mar 1, 2023
@danielroe danielroe deleted the feat/sourcemap-ignore branch March 1, 2023 15:48
@bmeurer
Copy link
Contributor

bmeurer commented Mar 2, 2023

Thanks folks! Now we'll also need to update rollup to 3.18.0 to include rollup/rollup#4877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants