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.

Sign up for GitHub

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

Jump to bottom

feat(node): Check for invalid url in node transport #6623

Merged
merged 3 commits into from Jan 5, 2023
Merged

feat(node): Check for invalid url in node transport #6623

merged 3 commits into from Jan 5, 2023

Conversation

Naddiseo
Copy link
Contributor

@Naddiseo Naddiseo commented Dec 28, 2022 •
edited

If the tunnel option will cause an error to be thrown, catch it, and give a warning.

Fixes GH-6381

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@Naddiseo
fix(nextjs): Check for invalid tunnel option on server
c04cf80 
If the tunnel option will cause an error to be thrown, catch it, and re-throw
with a better error message.


Fixes getsentryGH-6381
@lforst lforst self-requested a review December 28, 2022 14:35
lforst
lforst reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR! I left some comments on how we can integrate this change into the project so it aligns with our vision.

Could you let me know if you have the time to implement it? Otherwise, I will put it into our backlog.

packages/nextjs/src/index.server.ts Outdated Show resolved Hide resolved
packages/nextjs/src/index.server.ts Outdated Show resolved Hide resolved
packages/nextjs/src/index.server.ts Outdated Show resolved Hide resolved
@Naddiseo Naddiseo force-pushed the 6381-better-error-message branch 2 times, most recently from 440f7ac to b20f95a Compare January 3, 2023 18:09
@Naddiseo
- move tunnel check from @sentry/nextjs to @sentry/node
034fd7b 
- Use `console.warn` instead of `logger.warn`
- Do not re-throw the error
@lforst
Copy link
Member

lforst commented Jan 5, 2023

After looking at what you wrote here,

The tunnel option is not a valid URL. It must be a full URL including the protocol when using the Node transport.

I realized that the logic best lives in the node transport. I moved it there and added some tests. Sorry to stomp on your work like that but thank you very much for doing the foundational work and figuring everything out!!

@lforst lforst requested review from Lms24 and mydea January 5, 2023 09:49
@lforst lforst self-assigned this Jan 5, 2023
@lforst lforst enabled auto-merge (squash) January 5, 2023 09:55
@lforst lforst disabled auto-merge January 5, 2023 09:55
@lforst lforst changed the title fix(nextjs): Check for invalid tunnel option on server feat(node): Check for invalid url in node transport Jan 5, 2023
@lforst lforst enabled auto-merge (squash) January 5, 2023 09:56
@lforst lforst merged commit 0f9a3df into getsentry:master Jan 5, 2023
@lforst
Copy link
Member

lforst commented Jan 5, 2023

@Naddiseo Thank you for your contribution!

@Naddiseo
Copy link
Contributor Author

Naddiseo commented Jan 5, 2023

After looking at what you wrote here,

The tunnel option is not a valid URL. It must be a full URL including the protocol when using the Node transport.

I realized that the logic best lives in the node transport. I moved it there and added some tests. Sorry to stomp on your work like that but thank you very much for doing the foundational work and figuring everything out!!

No worries! I did think it belonged in node-transport, but saw that init function was were all the parameter checking was done.
Thank you for getting this through!

@Naddiseo Naddiseo deleted the 6381-better-error-message branch January 5, 2023 16:36
@shouze shouze mentioned this pull request May 2, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lforst lforst lforst left review comments

@mydea mydea mydea approved these changes

@Lms24 Lms24 Awaiting requested review from Lms24

Assignees

@lforst lforst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ERR_INVALID_URL for next config rewrite option
3 participants
@Naddiseo @lforst @mydea