Skip to content

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

Merged
lforst merged 3 commits into
getsentry:masterfrom
Naddiseo:6381-better-error-message
Jan 5, 2023
Merged

feat(node): Check for invalid url in node transport#6623
lforst merged 3 commits into
getsentry:masterfrom
Naddiseo:6381-better-error-message

Conversation

@Naddiseo

@NaddiseoNaddiseo commented Dec 28, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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

FixesGH-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.
FixesgetsentryGH-6381
@lforst
lforst self-requested a review December 28, 2022 14:35
lforst
lforst reviewed Jan 3, 2023
View reviewed changes

@lforstlforst left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadpackages/nextjs/src/index.server.ts Outdated
Comment threadpackages/nextjs/src/index.server.ts Outdated
Comment threadpackages/nextjs/src/index.server.ts Outdated
@Naddiseo
Naddiseoforce-pushed the 6381-better-error-message branch 2 times, most recently from 440f7ac to b20f95aCompareJanuary 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
@Naddiseo
Naddiseoforce-pushed the 6381-better-error-message branch from b20f95a to 034fd7bCompareJanuary 4, 2023 17:29
@lforst

lforst commented Jan 5, 2023

Copy link
Copy Markdown
Contributor

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 mydeaJanuary 5, 2023 09:49
@lforstlforst 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
@lforstlforst changed the title fix(nextjs): Check for invalid tunnel option on serverfeat(node): Check for invalid url in node transportJan 5, 2023
@lforst
lforst enabled auto-merge (squash) January 5, 2023 09:56
@lforst
lforst merged commit 0f9a3df into getsentry:masterJan 5, 2023
@lforst

lforst commented Jan 5, 2023

Copy link
Copy Markdown
Contributor

@Naddiseo Thank you for your contribution!

@Naddiseo

Naddiseo commented Jan 5, 2023

Copy link
Copy Markdown
ContributorAuthor

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
@shouzeshouze mentioned this pull request May 2, 2023
Closed
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@Lms24Lms24Awaiting requested review from Lms24
+1 more reviewer
@lforstlforstlforst left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@lforstlforst

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