Skip to content

chore: removes sourcemap-register from build step#696

Merged
andrialexandrou merged 2 commits into
mainfrom
aja/exclude-source-register
Apr 21, 2025
Merged

chore: removes sourcemap-register from build step#696
andrialexandrou merged 2 commits into
mainfrom
aja/exclude-source-register

Conversation

@andrialexandrou
Copy link
Copy Markdown
Contributor

@andrialexandrou andrialexandrou commented Apr 18, 2025

Towards https://github.com/actions/add-to-project/security/code-scanning/4
Closes https://github.com/github/planning-tracking/issues/2239

Open question here whether

  1. the sourcemap-register does anything re: improving source maps
  2. whether built maps are something that an end user would depend on in an actions context

Note that there's a step in CI that ignores the content generated in the sourcemap-register:

Copilot AI review requested due to automatic review settings April 18, 2025 20:51
@andrialexandrou andrialexandrou requested a review from a team as a code owner April 18, 2025 20:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • package.json: Language not supported

@wiinci
Copy link
Copy Markdown
Contributor

wiinci commented Apr 21, 2025

Let's wait on approvals from folks who might have more context around the sourcemap-register.js in actions 🙇

@camchenry
Copy link
Copy Markdown
Contributor

camchenry commented Apr 21, 2025

It looks like this is the relevant issue upstream: evanw/node-source-map-support#320. The issue itself seems pretty harmless, I doubt this would introduce a vulnerability given it only relates to the source maps.

It looks like the original intent of the package was to implement source map support when Node/V8 didn't support it, but the Chrome DevTools did. I don't think that is the case any longer, as it looks like support for source maps is built-in to Node now via the --enable-source-maps CLI option: https://nodejs.org/docs/v22.14.0/api/cli.html#--enable-source-maps.

I think that source maps could be helpful, as it might make the error messages more helpful by referencing the original source file name instead of the compiled file name. That said, I don't think removing this file should cause any significant impact as the worst case scenario is the error messages are perhaps slightly worse, but given how simple this library is it's not a big deal I think.

It's may also be worth reconsidering our usage of @vercel/ncc, as it seems to be less maintained these days. There are more modern bundlers/builders such as tsdown we could consider.

@andrialexandrou andrialexandrou added this pull request to the merge queue Apr 21, 2025
Merged via the queue into main with commit 5b1a254 Apr 21, 2025
5 checks passed
@andrialexandrou andrialexandrou deleted the aja/exclude-source-register branch April 21, 2025 18:32
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.

4 participants