-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(serverBareModulesPlugin): update require.resolve to use full import, update warning #3656
Conversation
…rt, update warning Signed-off-by: Logan McAnsh <logan@mcan.sh>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think we should probably have a test for this functionality (not just the warning, but that too). Could you implement that?
i agree, in my initial PR i wasn't sure about the best way to test for that being that a missing dependency would fail the build in esbuild.build, but i'll look into how others are doing that detection (and how we are for esm warnings) |
this test also covers installed modules as it uses @remix-run/node and @remix-run/react Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
🦋 Changeset detectedLatest commit: 09008c6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Logan McAnsh <logan@mcan.sh>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coolio 👍 Thanks!
🤖 Hello there, We just published version Thanks! |
Signed-off-by: Logan McAnsh logan@mcan.sh
Closes: #
Testing Strategy: