Skip to content

Conversation

@iamstarkov
Copy link

@iamstarkov iamstarkov commented Feb 16, 2025

should address issue described in #1253

I was also thinking to expose sourcemap base prefix as an ncc argument in case there are more bugs or edge cases and some people will require manual adjustment, but decided to start small

@iamstarkov iamstarkov changed the title fix: make sourcemap base prefix relative to the build file instead of default "../" (fixes #1253) fix: make sourcemap base prefix relative from outdir to the build file instead of static "../" (fixes #1253) Feb 16, 2025
@iamstarkov iamstarkov force-pushed the fix/sourcemap-base-prefix-1253 branch from 2464f3d to 7e2b6d3 Compare April 8, 2025 10:18
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks, can you add a test to confirm this works?

@iamstarkov
Copy link
Author

Thanks, can you add a test to confirm this works?

Yes, I'll give it a try

@iamstarkov
Copy link
Author

iamstarkov commented Apr 29, 2025

@styfle added relevant tests

3 unrelated integration tests fail for me locally both in my branch and in up to date main branch.

These are the tests and corresponding errors:

  • should execute "ncc run aws-sdk.js": Error: Cannot find module '@aws-sdk/client-s3'
  • should execute "ncc run binary-require.js": Error: Cannot find module './hello.node'
  • should execute "ncc run canvas.js": Error: Cannot find module '../build/Release/canvas.node'

I assume its because npm run build-test-binary fails due to missing distutils, which had been fixed in node-gyp 10, while ncc uses 9

@iamstarkov
Copy link
Author

how does it look?

@iamstarkov iamstarkov requested a review from styfle May 22, 2025 19:00
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Looks like tests are failing CI though.

Were you able to get tests passing locally?

@iamstarkov
Copy link
Author

iamstarkov commented May 23, 2025

Thanks for the PR!

Looks like tests are failing CI though.

Were you able to get tests passing locally?

last time around all but few unrelated tests were passing, see more in the prev comment #1254 (comment)

let me check now

@iamstarkov
Copy link
Author

I believe the test dont work no more, because suggested change broke them =)

@styfle
Copy link
Member

styfle commented May 23, 2025

I see. I guess we can try the old code but it didn't seem right. Would't it trim the wrong character if it didn't end with a /? 🤔

@iamstarkov
Copy link
Author

yes, probably. but I think this thing is a crutch. we should adjust webpack configuration so it generates sourcemaps relative to the source to begin with

@iamstarkov
Copy link
Author

I gonna give it a go

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.

2 participants