Skip to content

Conversation

@normanzb
Copy link
Contributor

@normanzb normanzb commented Nov 22, 2024

Change Summary

This PR avoids pathname overwriting when passing in an URLObject with all necessary props to generateTargetURL().

If the user passing in a URL object with all props required to construct a full URL, that means they want to go straight to there without modification. But current behaviour of generateTargetURL() overwrite its pathname by prepending the pathname of baseUrl

Merge Checklist

  • PR has a Changeset
  • PR includes documentation if necessary
  • PR updates the boilerplates if necessary

@vercel
Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
frames-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 25, 2024 10:00am
framesjs-debugger ❌ Failed (Inspect) Nov 25, 2024 10:00am
framesjs-examples ❌ Failed (Inspect) Nov 25, 2024 10:00am

@stephancill stephancill changed the base branch from main to dev November 22, 2024 15:10
}

function isUrlObjectComplete(urlObject: UrlObject): boolean {
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also check if the values are set right? URLObject is just saying that there are properties but their values can be empty right?

Also wouldn't it make sense to also allow URL class instance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tha last point about URL instance is for generateTargetUrl function. baseUrl is typed like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

URLObject is already compatible with URL, the user is able to pass an URL object. Also to add URL explicitly the whole call stacks will be updated. as it is gaining nothing I think let's leave that part out?

I've updated value check into isUrlObjectComplete, pls have a look @michalkvasnicak , thanks!

@normanzb normanzb merged commit 70af4ca into dev Nov 25, 2024
6 of 8 checks passed
@normanzb normanzb deleted the fix/pass-in-full-url branch November 25, 2024 12:38
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.

4 participants