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: site.entry-point should not be a hard deprecation #843

Merged
merged 1 commit into from
May 3, 2022

Conversation

threepointone
Copy link
Contributor

To make migration of v1 projects easier, Sites projects should still work, including the entry-point field (which currently errors out). This enables site.entry-point as a valid entry point, with a deprecation warning.


I also added an example repo, which is simply the output of wrangler generate --site

@changeset-bot
Copy link

changeset-bot bot commented Apr 25, 2022

🦋 Changeset detected

Latest commit: 7d81a81

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 Apr 25, 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/2266650886/npm-package-wrangler-843

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

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

Or you can use npx with this latest build directly:

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

Copy link
Contributor

@caass caass 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 pretty good.

The example app is just copied directly from wrangler 1, right?

The messaging is a little confusing to me. The deprecation warning is that site.entry-point is no longer used, but actually it seems like we do actually respect it? If that's the case, then maybe we should instead encourage people to move site.entry-point to main?

Like basically my nit is that the messaging implies we're ignoring site.entry-point but actually...we're not. I think..? Other than that though, looks good :)

packages/wrangler/src/entry.ts Outdated Show resolved Hide resolved
@threepointone
Copy link
Contributor Author

Good point about the messages, I reused what it already was. I'll rewrite it.

@threepointone
Copy link
Contributor Author

tbh I did such a shoddy job with this PR, noticing that I just commented out a couple of tests I should have implemented. Working today on making this a lot better.

@threepointone threepointone force-pushed the site-entry-point-deprecation branch 2 times, most recently from 7cd70da to 398d964 Compare May 3, 2022 11:22
@threepointone threepointone requested a review from caass May 3, 2022 11:23
@threepointone
Copy link
Contributor Author

threepointone commented May 3, 2022

Alright, let's try this again. Messaging better aligned with our other deprecations, better tests. What do you think?

@threepointone threepointone force-pushed the site-entry-point-deprecation branch 2 times, most recently from ee931ac to a40e9d3 Compare May 3, 2022 11:41
Copy link
Contributor

@caass caass left a comment

Choose a reason for hiding this comment

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

Nice! Approving with some minor nits that you can do if you want, and a request to create a follow-up issue for normalizing windows paths in tests

.changeset/afraid-oranges-happen.md Outdated Show resolved Hide resolved
packages/wrangler/src/__tests__/configuration.test.ts Outdated Show resolved Hide resolved
packages/wrangler/src/__tests__/configuration.test.ts Outdated Show resolved Hide resolved
packages/wrangler/src/__tests__/configuration.test.ts Outdated Show resolved Hide resolved
@threepointone threepointone force-pushed the site-entry-point-deprecation branch 3 times, most recently from 2ee5072 to 148d3b8 Compare May 3, 2022 22:12
To make migration of v1 projects easier, Sites projects should still work, including the `entry-point` field (which currently errors out). This enables `site.entry-point` as a valid entry point, with a deprecation warning.
@threepointone
Copy link
Contributor Author

Thanks for the reviews and the patience peeps. Landing this now.

@threepointone threepointone merged commit da12cc5 into main May 3, 2022
@threepointone threepointone deleted the site-entry-point-deprecation branch May 3, 2022 22:20
@github-actions github-actions bot mentioned this pull request May 3, 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.

None yet

3 participants