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.

Sign up for GitHub

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

Jump to bottom

Add x_google_ignoreList (Ignore-listing code) support to sourcemaps #1066

Open
0xdevalias opened this issue Dec 2, 2023 · 9 comments
Open

Add x_google_ignoreList (Ignore-listing code) support to sourcemaps #1066

0xdevalias opened this issue Dec 2, 2023 · 9 comments

Comments

@0xdevalias
Copy link

0xdevalias commented Dec 2, 2023 •
edited
Loading

Introduce the x_google_ignoreList extension in the sourcemaps generated by this project. This will facilitate a more streamlined debugging experience in Chrome (and other supporting browsers) by automatically filtering out framework and dependency code.

Benefit

Implementing x_google_ignoreList in the sourcemaps will align this project with modern web development practices, offering a more focused and efficient debugging experience for developers using Chrome DevTools. This change will particularly benefit those who regularly engage in debugging complex applications with numerous dependencies.

Background Context

Originally posted by @0xdevalias in facebook/react#27774 (comment)

See Also
This was referenced Dec 2, 2023
Closed
Open
Closed
Open
@rschristian
Copy link
Collaborator

Microbundle is a tool for building libraries, not apps. So again, wrong layer as this should be implemented in the tools that consume Microbundle's output.

Also, I don't think opening these copy/pasted issues everywhere is going to get you the desired reaction.

@rschristian rschristian closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
@0xdevalias
Copy link
Author

0xdevalias commented Dec 2, 2023 •
edited
Loading

Microbundle is a tool for building libraries, not apps. So again, wrong layer as this should be implemented in the tools that consume Microbundle's output.

@rschristian This context from the other thread that explains why I opened it here:

Fair enough. Well you may or may not find this issue relevant there then, since it's a bundler (though I'm not really sure how a thing like this that should be configurable would be exposed through a 'no config' tool):

Originally posted by @0xdevalias in preactjs/preact#4225 (comment)

Also, I don't think opening these copy/pasted issues everywhere is going to get you the desired reaction.

@rschristian I'm not sure what you're assuming my 'desired reaction' is here. I'm trying to get a useful feature implemented across the major web frameworks. It's copy pasted because there is no point individually re-writing the same context and details every time since it will be exactly the same. They are cross-linked so that it's easier to find the requests, any relevant comments/context, and implementation specifics between the frameworks to reduce the effort of having to 'start from scratch' for each.

@rschristian
Copy link
Collaborator

This context from the other thread that explains why I opened it here:

Fair enough. Well you may or may not find this issue relevant there then, since it's a bundler

Per the tag line, this is a tool for modules, not apps.

I get why you opened it but, like Preact's repo, it doesn't fit here. Everything Microbundle outputs is meant for use in other tools really, tools which can and should control this setting themselves.

I'm not sure what you're assuming my 'desired reaction' is here.

Your desired reaction would be a positive one, I'd imagine.

The problem is that this comes off as incredibly spammy, especially as it's mistargeted. Copy/pasting issues on a bunch of repos just isn't a good way to go about things. I fully understand you mean well but this isn't the way to do it.

@0xdevalias
Copy link
Author

0xdevalias commented Dec 2, 2023 •
edited
Loading

Copy/pasting issues on a bunch of repos just isn't a good way to go about things. I fully understand you mean well but this isn't the way to do it.

Genuine question: Ignoring the 'mistargeted' aspect (assuming that's true and I've just misunderstood); if someone has a desire to have a useful ecosystem feature considered across a number of projects in a related category (eg. web frameworks/'UI libraries'/etc; what would you consider to be 'the way to do it'? As this seemed like the most useful and direct method I could think of for doing so.

@rschristian
Copy link
Collaborator

I still think this should be done on the bundler/app/user side of things, rather than in libraries, but if you want to contribute a fix, go for it.

if someone has a desire to have a useful ecosystem feature considered across a number of projects in a related category (eg. web frameworks/'UI libraries'/etc; what would you consider to be 'the way to do it'?

Frankly, you shouldn't.

If you want to see a feature land in a project you actually use, then sure, go ahead and open an issue, perhaps with a few "prior art" links. But copy/pasting the same request across multiple projects just because you read about a new feature can come off as quite demanding on OSS maintainers. Imagine if everyone was doing this.

But if did insist on doing it, you should be shortly opening a PR yourself alongside that issue request (an example of this was when some NPM people went around correcting popular packages' "repository" fields as many were invalid or using perhaps ill-spec'd versions). Asking for the entire ecosystem to change, without investing your own time in landing it, is simply not healthy in OSS.

@rschristian rschristian reopened this Dec 2, 2023
@0xdevalias
Copy link
Author

I still think this should be done on the bundler/app/user side of things, rather than in libraries, but if you want to contribute a fix, go for it.

@rschristian I'll have to dig deeper into the code/etc here before I could answer this one way or another myself, but in case that doesn't turn up a good answer, maybe I can pre-empt getting stuck and ask ahead of time.

Given microbundle is a 'Zero-configuration bundler', my assumption was that was literal and there was no way for a downstream user to configure it; but quickly skimming the README it seems there may be a few options available. Do you have a preference for how I implement this? Should it be a default on, and/or added as a new configurable option, etc?

If you want to see a feature land in a project you actually use, then sure, go ahead and open an issue, perhaps with a few "prior art" links. But copy/pasting the same request across multiple projects just because you read about a new feature can come off as quite demanding on OSS maintainers. Imagine if everyone was doing this.

@rschristian (Mini essay here, didn't intend it to be so long; definitely no pressure/expectation to read it all/reply in detail)

Somewhat off topic 'mini essay' response about my process

I understand your view here, though it's making some assumptions that aren't true in this case. It's not just that I 'read about a new feature' and wanted to spam it across a bunch of projects, but because that new feature would be specifically useful in my downstream workflows (eg. web app analysis), and that it being adopted across the top web frameworks/UI libraries would mean that majority of apps I come across would then support it by default.

As far as it coming off as demanding, that's definitely not my intention. I don't open issues to 'demand'; I raise a thing (bug, feature, etc) that I think would benefit the project, as an invitation to a conversation/collaboration around the thing being raised. Ultimately if it's not something a project is willing to implement/accept, that's fair enough, it's not my place to demand it (though as you no doubt inferred from our conversation on the other issue, my neurodiverse brain wants to understand the "why can't it" if the basis of a 'no' is simply more than "I don't want that in my project"; as I've found that often people can quickly jump to a conclusion that doesn't always hold true under further exploration)

Then, depending on what the thing being raised is, my time/energy (and whether my ADHD brain remembers to), I will then often try and dig into some of the deeper specifics as relevant to that specific project, and add further context on that issue for how I think it could be implemented/etc; though as I am often not deeply familiar with the projects, it can take more time/effort to figure out those specifics, and I like to have somewhere created to 'put those learnings' when I do (thus the issue). Also sometimes a maintainer jumps in before I get a chance to look at those deeper things and has ideas/pointers for where to look to implement it (or if it's something they won't accept regardless, and thus not worth me looking deeper into it).

Then finally, based on that analysis/context, either I or another contributor should be able to more quickly/easily make the specific changes required to implement/fix the thing.

I guess I sort of see it as a 'staged process', with each stage aiming to contribute some value, even if I don't manage to keep my brain/memory focussed on it long enough to see it through to the fully completed solution.

But if did insist on doing it, you should be shortly opening a PR yourself alongside that issue request

@rschristian That's totally fair. As I sort of inferred in my 'mini-essay' above, I often don't want to jump straight to a PR before there has been at least some level of "yes, sounds good" from a project; as it's not worth sinking my time/energy into implementing it if the project will just turn around and be like "no, we don't want that".

Asking for the entire ecosystem to change, without investing your own time in landing it, is simply not healthy in OSS.

@rschristian nods agreed.

@rschristian
Copy link
Collaborator

Do you have a preference for how I implement this? Should it be a default on, and/or added as a new configurable option, etc?

Sorry, meant to say that enabling it is fine, no need for config.

I don't love that package maintainers will need to tell their consumers that they'll have to alter their browser's settings if they want the old behavior (as this seems to be enabled by default in Chrome), but we have too many flags here as-is, IMO.

I don't open issues to 'demand'; I raise a thing (bug, feature, etc) that I think would benefit the project, as an invitation to a conversation/collaboration around the thing being raised.

I often don't want to jump straight to a PR before there has been at least some level of "yes, sounds good" from a project; as it's not worth sinking my time/energy

I get that, but unfortunately collaboration like this demands a time investment from someone: either the (potential) contributor doing work up front or the maintainers fielding questions. I know it seems like a quick thing but many of us have another 50+ repos we're fielding questions in (and even more that we're neglecting) on top of work, personal life, etc.

But again, that's just me. Opening issues for discussion/one-offs are a-okay, but if you're trying to standardize an entire ecosystem, personally, it comes off a lot more positive if you're making some initial investment to make that happen. A hacky, post-build injection in Preact's source maps (for this issue) would've been a fine starting point, for example.

@0xdevalias
Copy link
Author

enabling it is fine, no need for config

@rschristian Ok, thanks for clarifying :)

I don't love that package maintainers will need to tell their consumers that they'll have to alter their browser's settings if they want the old behavior (as this seems to be enabled by default in Chrome)

@rschristian nods, understandable. Though it sounds like in many cases a new default coming in Chrome 120 would have a similar impact on a majority of bundled code anyway:

And I guess for a lot of 'end-users' of libraries, in a majority of cases they aren't ever going to need to switch back to the old behaviour, as the internals of a lib aren't often super relevant to debugging web app code.

I'd expect it to theoretically be more of a potential concern to library maintainers themselves, but then they're probably not developing on the bundled/minimised version of their lib anyway, so this shouldn't impact them there either.

I get that, but unfortunately collaboration like this demands a time investment from someone: either the (potential) contributor doing work up front or the maintainers fielding questions. I know it seems like a quick thing but many of us have another 50+ repos we're fielding questions in (and even more that we're neglecting) on top of work, personal life, etc.

@rschristian nods that's totally fair, and I can understand and relate to that feel.

Opening issues for discussion/one-offs are a-okay, but if you're trying to standardize an entire ecosystem, personally, it comes off a lot more positive if you're making some initial investment to make that happen. A hacky, post-build injection in Preact's source maps (for this issue) would've been a fine starting point, for example.

@rschristian nods fair enough, I can definitely take that feedback on board :)

@0xdevalias
Copy link
Author

0xdevalias commented Dec 4, 2023 •
edited
Loading

Tracing the right place to make these changes

The change to be made is in rollup's output config:

  • https://rollupjs.org/configuration-options/#output-sourcemapignorelist

    • output.sourcemapIgnoreList
      Type: boolean | (relativeSourcePath: string, sourcemapPath: string) => boolean
      A predicate to decide whether or not to ignore-list source files in a sourcemap, used to populate the x_google_ignoreList source map extension.
      When you don't specify this option explicitly, by default it will put all files with node_modules in their path on the ignore list. You can specify false here to turn off the ignore-listing completely.

Based on the description of that rollup config option, since microbundle is designed for libraries, I believe it should be fine to just pass true here, and ignore-list all of the library files.

Based on the above exploration of microbundle's code, it looks like the outputOptions config passed to rollup are defined here:

So the entire change to implement this is probably just:

  // ..snip..

  outputOptions: {
    // ..snip..
    sourcemap: options.sourcemap,
+   sourcemapIgnoreList: true,
    // ..snip..
  },

  // ..snip..

After making that, I was getting some warnings while running npm test:

console.warn
    Unknown output options: sourcemapIgnoreList. Allowed options: amd, assetFileNames, banner, chunkFileNames, compact, dir, dynamicImportFunction, entryFileNames, esModule, exports, extend, externalLiveBindings, file, footer, format, freeze, globals, hoistTransitiveImports, indent, inlineDynamicImports, interop, intro, manualChunks, minifyInternalExports, name, namespaceToStringTag, noConflict, outro, paths, plugins, preferConst, preserveModules, preserveModulesRoot, sourcemap, sourcemapExcludeSources, sourcemapFile, sourcemapPathTransform, strict, systemNullSetters

output.sourcemapIgnoreList was added to rollup in 3.16.0 (2023-02-17):

microbundle is using a super outdated version of rollup (2.35.1) from 2020-12-14:

So before we can implement this change, we'll need to upgrade to a modern version of rollup.

I'll work on getting that change implemented and hopefully put up a PR for it shortly.

Edit: Maybe not so shortly, as it looks like microbundle's version of rollup is super outdated (using 2.35.1, and this flag isn't supported till 3.16.0. Opened a new issue to track this:

@0xdevalias 0xdevalias mentioned this issue Dec 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@0xdevalias @rschristian