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 silent failure when browser open doesn't work #263

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

jkriss
Copy link
Contributor

@jkriss jkriss commented Jan 18, 2022

The solution in #211 doesn't work, because the error is thrown by the child process returned by the open function, not by the open itself.

Tested with GitHub Codespaces.

It's the returned child process that throws an error, not the launching function itself
@changeset-bot
Copy link

changeset-bot bot commented Jan 18, 2022

🦋 Changeset detected

Latest commit: 50b4aa4

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

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

This is looking good! Please add a changeset, as per the instructions. Also, run npm run prettify at the root of the package to pass the build error.

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

This looks good, thank you very much for your PR!

@threepointone threepointone merged commit 402c77d into cloudflare:main Jan 18, 2022
@github-actions github-actions bot mentioned this pull request Jan 18, 2022
@threepointone
Copy link
Contributor

You can try the fix by using wrangler@alpha. We'll eventually do a beta release, probably early next week.

@jkriss
Copy link
Contributor Author

jkriss commented Jan 18, 2022

Nice, my fix works with wrangler@alpha. I did, however, get this error with alpha:

Cannot find package '@miniflare/core' imported from /workspaces/finite.photos/node_modules/wrangler/wrangler-dist/cli.js Running npm i -D @miniflare/core fixed it.

Looks like that came in with #186 , but the @miniflare/core package wasn't added to package.json.

@threepointone
Copy link
Contributor

Thanks for the catch! This might be because of #264. I'll fix this tomorrow.

@petebacondarwin
Copy link
Contributor

miniflare/core is a dependency of miniflare, so it should not need to be directly added to the wrangler package.json nor your own project. I just updated a project to wrangler@alpha and all these deps were there. I wonder if your node_modules just got a bit corrupted when you updated to alpha?

@jkriss
Copy link
Contributor Author

jkriss commented Jan 19, 2022

Yep, sure enough. Sorry for the false alarm!

threepointone added a commit that referenced this pull request Feb 8, 2022
We open browser windows for a few things; during `wrangler dev`, and logging in. There are environments where this doesn't work as expected (like codespaces, stackblitz, etc). This fix simply logs an error instead of breaking the flow. This is the same fix as #263, now applied to the rest of wrangler.
threepointone added a commit that referenced this pull request Feb 9, 2022
We open browser windows for a few things; during `wrangler dev`, and logging in. There are environments where this doesn't work as expected (like codespaces, stackblitz, etc). This fix simply logs an error instead of breaking the flow. This is the same fix as #263, now applied to the rest of wrangler.
threepointone added a commit that referenced this pull request Feb 9, 2022
We open browser windows for a few things; during `wrangler dev`, and logging in. There are environments where this doesn't work as expected (like codespaces, stackblitz, etc). This fix simply logs an error instead of breaking the flow. This is the same fix as #263, now applied to the rest of wrangler.
@github-actions github-actions bot mentioned this pull request Feb 9, 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.

3 participants