-
Notifications
You must be signed in to change notification settings - Fork 774
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
wrangler pages publish #790
wrangler pages publish #790
Conversation
sidharthachatterjee
commented
Apr 13, 2022
•
edited by GregBrimble
Loading
edited by GregBrimble
- changeset
- tests
🦋 Changeset detectedLatest commit: 673c2a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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/2226663394/npm-package-wrangler-790 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/790/npm-package-wrangler-790 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2226663394/npm-package-wrangler-790 dev path/to/script.js |
6594597
to
0b41ff7
Compare
0b41ff7
to
5da43f0
Compare
Will this change include the ability to push previews? docs |
5eb8c18
to
0b32c13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished off @sidharthachatterjee 's work, so my stamp is a little hollow. If you wouldn't mind taking a look at the stuff I've done and also stamp, @sidharthachatterjee , we can get this in.
@GregBrimble Thank you for finishing this up! Looks great. |
@@ -36,6 +36,7 @@ | |||
"cli" | |||
], | |||
"dependencies": { | |||
"blake3-wasm": "^2.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you tell me why we use blake3-wasm
here, and not xxhash
which we already use for sites? If possible, I'd like to use xxhash everywhere, unless there's a reason I'm not aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use Blake3 in Pages currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did come across a pure-JS implementation later on for something else, so we could swap that in, in place of the WASM, if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, so that hashes match what Pages already does internally. But since the projects published via API wouldn't be compared with "native" Pages projects, would the choice of hasher here matter? Just want to make sure we're intentional here with this choice, because changing later would invalidate all previous projects (if I'm understanding correctly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a possible future where you can upload to an existing project, so yeah, we need to be consistent with how files are hashed today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight, that's a good reason.