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!: Remove resolve dependency which increases min Node version to v8.10.0 #104

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jan 30, 2025

This PR removes resolve as a dependency and as a result changes our minimum supported Node version from v8.0.0 to v8.10.0.

@timfish
Copy link
Contributor Author

timfish commented Feb 3, 2025

Will this break Node Single Executable Apps?

@trentm
Copy link
Contributor

trentm commented Feb 4, 2025

Will this break Node Single Executable Apps?

I was going to (perhaps naively) suggest that RITM in a SEA app should no-op for non-core modules.

https://nodejs.org/api/single-executable-applications.html#requireid-in-the-injected-main-script-is-not-file-based suggests that a file-based require can be obtained, however. So I guess we'll need to feel this out and add tests.

I suppose that could be considered separate work and not necessarily a breaking change? Supporting SEA apps at all was accidental.

@timfish
Copy link
Contributor Author

timfish commented Feb 4, 2025

Yeah it looks like SEA apps were working out of luck before and we broke them with my change for 7.5.0.

So we should merge #106 and then get some tests in place before merging this PR!

@BYK
Copy link
Contributor

BYK commented Feb 4, 2025

Not sure if I understand the implications fully here but the SEA application I'm working on uses Sentry Node SDK to instrument itself (not just errors) which then relies on this package. If that stops working, that would be quite disappointing as I think we should be able to instrument SEA apps?

@timfish
Copy link
Contributor Author

timfish commented Feb 4, 2025

If that stops working, that would be quite disappointing as I think we should be able to instrument SEA apps?

We intend to add SEA tests before merging this PR to ensure we retain compatibility.

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.

3 participants