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(sourcemaps): Check for release or debug ids before upload #1561

Merged
merged 9 commits into from
Apr 5, 2023

Conversation

loewenheim
Copy link
Contributor

This should take care of #1547.

Unfortunately, it requires a significant refactoring of SourceMapProcessor. Parts of add_sourcemap_references and inject_debug_ids have been extracted into a new method collect_sourcemap_references that collects the sourcemap files referenced from minified sources into a map saved in the processor. This method is called from flush_pending_sources. There is still some duplication of the logic in unpack_ram_bundle which could probably be subsumed into this, but I'm not confident I can do it correctly.

I decided to bail with an error if no files have debug ids and use warn! if only some of them do. I think these choices and the error messages are still up for debate, though.

Also: any idea how we could test this?

@loewenheim loewenheim self-assigned this Apr 3, 2023
@loewenheim loewenheim linked an issue Apr 3, 2023 that may be closed by this pull request
@iambriccardo
Copy link
Member

I would test it in the following way:

  1. Create a new bundle
  2. Run the upload command with the new flag
  3. Expect an error
    --
  4. Create a new bundle
  5. Run the inject command
  6. Manually remove debug ids from one or more files
  7. Run the upload command with the new flag
  8. Expect a warning
    --
  9. Create a new bundle
  10. Run the upload command with the new flag and release
  11. Expect no error

Copy link
Member

@iambriccardo iambriccardo left a comment

Choose a reason for hiding this comment

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

LGTM

src/utils/sourcemaps.rs Outdated Show resolved Hide resolved
Co-authored-by: Riccardo Busetti <riccardob36@gmail.com>
@loewenheim
Copy link
Contributor Author

Locally running this has raised a question: what should happen if a minified file points to a source file that can't be found on the local file system? Should that raise a warning for a missing debug id?

@iambriccardo
Copy link
Member

Locally running this has raised a question: what should happen if a minified file points to a source file that can't be found on the local file system? Should that raise a warning for a missing debug id?

If you mean that it points to an invalid location for the sourcemap, I would also consider this as an "injection" failure and we could warn the user.

@kamilogorek
Copy link
Contributor

I know those are fixtures, but what's inside them is mostly irrelevant to us. Can we make them smaller than 50k LoC? 😅

@loewenheim
Copy link
Contributor Author

I know those are fixtures, but what's inside them is mostly irrelevant to us. Can we make them smaller than 50k LoC? 😅

Fair point, yes ^^

@loewenheim loewenheim merged commit 1b4c0d0 into master Apr 5, 2023
@loewenheim loewenheim deleted the feat/release-or-debugids branch April 5, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sourcemaps upload: require release or debug ids
3 participants