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

Rename unstable_dev() to dev() #2471

Closed
wants to merge 4 commits into from
Closed

Rename unstable_dev() to dev() #2471

wants to merge 4 commits into from

Conversation

rozenmd
Copy link
Contributor

@rozenmd rozenmd commented Dec 30, 2022

What this PR solves / how to test:

This PR resolves #1420

Associated docs issues/PR:

  • TODO

Author has included the following, where applicable:

  • Tests
  • Changeset

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Fixes #1420

@rozenmd rozenmd requested review from a team as code owners December 30, 2022 16:25
@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2022

🦋 Changeset detected

Latest commit: 76a15be

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

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

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 Dec 30, 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.devprod.cloudflare.dev/runs/3807921413/npm-package-wrangler-2471

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

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2471/npm-package-wrangler-2471

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3807921413/npm-package-wrangler-2471 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3807921413/npm-package-cloudflare-pages-shared-2471

@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #2471 (76a15be) into main (9728245) will increase coverage by 0.04%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2471      +/-   ##
==========================================
+ Coverage   72.13%   72.18%   +0.04%     
==========================================
  Files         156      156              
  Lines        9720     9717       -3     
  Branches     2546     2544       -2     
==========================================
+ Hits         7012     7014       +2     
+ Misses       2708     2703       -5     
Impacted Files Coverage Δ
packages/wrangler/src/dev.tsx 84.81% <ø> (ø)
packages/wrangler/src/pages/dev.tsx 19.02% <50.00%> (ø)
packages/wrangler/src/api/dev.ts 70.37% <75.00%> (+1.94%) ⬆️
packages/wrangler/src/api/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/git-client.ts 81.25% <0.00%> (+4.16%) ⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts 100.00% <0.00%> (+5.26%) ⬆️

@rozenmd
Copy link
Contributor Author

rozenmd commented Dec 30, 2022

We did it @threepointone

We did it joe gif

@penalosa
Copy link
Contributor

penalosa commented Jan 3, 2023

Just bringing up this point from the prior discussion:

#2315 (reply in thread)
I definitely vote we have like two properties stabilised (port and script, a path relative to where it's called), and mark everything else as experimental to give the most flexibility

From a quick read of this PR, it looks like there's an experimental property, but most things aren't part of it:
Screenshot 2023-01-03 at 13 34 30

I think this will limit the future of Wrangler as an API, and it might be worth considering marking more things as experimental. I don't have a full set of things that might change, but as some quick examples:

  • dev() currently takes a path from which it will read the config. If this is going to be flexible for multiple use cases, dev() probably shouldn't know about a config file, it should just take a config object (think of a WfP customer building a CLI, for instance, or indeed Wrangler supporting both TOML and JSON).
  • Similarly for auth handling—AFAIK dev() currently handles Oauth—but this should probably be handled at the wrapping CLI layer (i.e. in Wrangler proper), and dev() should take API keys.

I don't know if these need to block unstable_dev -> dev, but I do think they need more thinking about, and so to allow for that in the future, dev() should mark more options as experimental (I would would lean towards all of them, to be honest, except maybe port)

cc @mrbbot

@rozenmd
Copy link
Contributor Author

rozenmd commented Jan 3, 2023

I would would lean towards all of them, to be honest, except maybe port

@penalosa At that point, what's the difference between an experimental object containing the entire API, and not promoting unstable_dev to dev? We have to commit to the code we release at some point 😅

@JacobMGEvans
Copy link
Contributor

I would would lean towards all of them, to be honest, except maybe port

@penalosa At that point, what's the difference between an experimental object containing the entire API, and not promoting unstable_dev to dev? We have to commit to the code we release at some point 😅

I agree with Max here, since the API is directly utilizing the implementations of the CLI side and they are not "experimental" it doesn't make much since they would be "experimental" here. The API should reflect the current CLI implementation, eventually things will change, however we had talked about the CLI being implemented with the API which would be major/breaking changes to API would be to the CLI as well.

I also think making everything "experimental" in API options shows a lack of confidence in current implementations that those options represent in CLI as they are now.

@mrbbot
Copy link
Contributor

mrbbot commented Nov 6, 2023

Hey! 👋 Given the work we're doing to rearchitect Wrangler as a library (see #4175), I think it makes sense to close this PR. We aren't going to be stabilising unstable_dev() in its current form, so we don't want to rename this to dev().

@mrbbot mrbbot closed this Nov 6, 2023
@rozenmd
Copy link
Contributor Author

rozenmd commented Nov 6, 2023

Cool, fair enough - are there plans to write an issue to explain #4175? Feels a bit opaque from the outside.

@lrapoport-cf
Copy link
Contributor

hey @rozenmd, apologies for the delay! we've created a milestone for the wrangler as a library work that includes a summary issue for the work (#4730). we'll be tracking related PRs against that issue and within the milestone moving forward :)

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.

Promote unstable_dev() to dev()
5 participants