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

fix: strip leading */*. from routes when deducing a host for dev #1018

Merged
merged 1 commit into from
May 16, 2022

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented May 16, 2022

When given routes, we use the host name from the route to deduce a zone id to pass along with the host to set with dev session. Route patterns can include leading */*., which we don't account for when deducing said zone id, resulting in subtle errors for the session. This fix strips those leading characters as appropriate.

Fixes #1002

@changeset-bot
Copy link

changeset-bot bot commented May 16, 2022

🦋 Changeset detected

Latest commit: 9f5aa23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2022

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2334052220/npm-package-wrangler-1018

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1018/npm-package-wrangler-1018

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2334052220/npm-package-wrangler-1018 dev path/to/script.js

@threepointone threepointone force-pushed the strip-leading-asterisk branch from e7beb4f to 943ed76 Compare May 16, 2022 07:12
@threepointone
Copy link
Contributor Author

@JacobMGEvans @petebacondarwin What's happening here, I think, is that node_modules is being restored from cache, which blows away the symlinks that npm install has setup for workspaces. I dunno why it hasn't happened for mac/ubuntu. But this seems like a blocker. Can we revert the actions caching changes until we figure that out? (unless you have a better solution for my problem?)

@threepointone threepointone requested a review from caass May 16, 2022 12:42
@petebacondarwin
Copy link
Contributor

Ah yes. I think when linux-like OSes zip up files they keep the symlinks but these are lost on Windows.
Maybe this didn't come up when we were testing, because re-running jobs might have just used the same workspace?

One option might be to run npm i if the node_modules were recovered from cache, since that might re-instantiate the links without actually doing any new installation?

@threepointone threepointone force-pushed the strip-leading-asterisk branch from 943ed76 to 4d619e8 Compare May 16, 2022 13:53
@threepointone
Copy link
Contributor Author

omg I added npm install and look at this error 😳😳😳 https://github.com/cloudflare/wrangler2/runs/6453430257?check_suite_focus=true

@threepointone threepointone force-pushed the strip-leading-asterisk branch from 4d619e8 to 91548aa Compare May 16, 2022 17:31
@threepointone
Copy link
Contributor Author

Ok, looks good now!

@threepointone threepointone force-pushed the strip-leading-asterisk branch from 91548aa to 81170a9 Compare May 16, 2022 18:52
Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

LGTM! regex one-liner backed by tests.

@threepointone
Copy link
Contributor Author

I have regex101.com open every day lol

When given routes, we use the host name from the route to deduce a zone id to pass along with the host to set with dev `session`. Route patterns can include leading `*`/`*.`, which we don't account for when deducing said zone id, resulting in subtle errors for the session. This fix strips those leading characters as appropriate.

Fixes #1002
@threepointone threepointone force-pushed the strip-leading-asterisk branch from 81170a9 to 9f5aa23 Compare May 16, 2022 19:02
@threepointone threepointone merged commit cd2c42f into main May 16, 2022
@threepointone threepointone deleted the strip-leading-asterisk branch May 16, 2022 19:10
@github-actions github-actions bot mentioned this pull request May 16, 2022
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.

bug: leading */*/. in routes causes bugs in wrangler dev
3 participants