Conversation
|
dario-piotrowicz
force-pushed
the
dario/autoconfig-types
branch
from
c6221c1 to
8ad25e7
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| "framework": Static { | ||
| "autoConfigSupported": true, | ||
| "configurationDescription": undefined, | ||
| "id": "static", | ||
| "name": "Static", | ||
| }, |
There was a problem hiding this comment.
Note: all the changes in this PR around Static being used are actually correctly representing the current behavior of autoconfig and there are no behavioral changes being introduced.
| `); | ||
| }); | ||
|
|
||
| it("should omit the framework entry when they it is not part of the details object", () => { |
There was a problem hiding this comment.
we might want to implement this, but currently we always get the 'Static' framework
| // If wranglerConfigToWrite is null we don't know whether there is server side code, we default to assume that there is | ||
| wranglerConfigToWrite === null || |
There was a problem hiding this comment.
wranglerConfigToWrite cannot be null, this change was incorrectly introduced in #12190
use `.at(0)`
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| configured: boolean; | ||
| /** Details about the detected framework (if any) */ | ||
| framework?: Framework; | ||
| /** Details about the detected framework. It can be a JS framework or 'Static' if no actual JS framework is used. */ |
There was a problem hiding this comment.
nit: "Static" sounds like a framework name to me, can you think of another name? StaticHtml/StaticAssets/StaticFiles ?
There was a problem hiding this comment.
yeah I agree... we have discussed this and haven't come up with anything better for now, I can open a followup PR to improve that if you want, but ideally I'd keep this PR as is (non-behavioral refactoring only)
| export type AutoConfigDetailsForConfiguredProject = Optional< | ||
| AutoConfigDetailsBase, | ||
| // If AutoConfig detects that the project is already configured it's unnecessary to check | ||
| // what framework is being used, so in this case `framework` is optional | ||
| "framework" | ||
| > & { | ||
| configured: true; | ||
| }; | ||
|
|
||
| export type AutoConfigDetailsForNonConfiguredProject = AutoConfigDetailsBase & { | ||
| configured: false; | ||
| }; | ||
|
|
||
| export type AutoConfigDetails = | ||
| | AutoConfigDetailsForConfiguredProject | ||
| | AutoConfigDetailsForNonConfiguredProject; | ||
|
|
There was a problem hiding this comment.
Question: It is really valuable to have separate AutoConfigDetails / AutoConfigDetailsForNonConfiguredProject / AutoConfigDetailsForConfiguredProject ?
Sounds like a lot of code for something simple. Myabe it is worth but maybe it is overkill.
I have look at the details in great depth and let you decide.
There was a problem hiding this comment.
That's the whole reason as to why I opened the PR 😅
I do think that this makes things better since thanks to this we can have more strict/precise types in the code and avoids the need to assert things or use unnecessary optional chaining.
So for me, this makes, as you pointed out, the type a bit more complex, but then it simplifies and makes more robust the autoconfig code in various places, it is a bit of a tradeoff 🙂
4 participants
As I've mentioned here: #12190 (comment)
I've noticed that the autoconfig types could be improved, so that's what this PR is for, it contains some minor changes to make the autoconfig types more correct/useful. Also removing optional chaining operations that are actually not necessary.
A picture of a cute animal (not mandatory, but encouraged)