-
-
Notifications
You must be signed in to change notification settings - Fork 77
feat(cloudflare): Enable update of the wrangler file #1149
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
Conversation
|
dc8f14c to
b049fce
Compare
32f962a to
63ca941
Compare
63ca941 to
31a46c6
Compare
Lms24
left a comment
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.
Nice!
Toml parsers are not quite popular and are not really feature complete, that is why toml files would loose all comments, once we update them (another option would be to not support .toml at all and give only a hint)
If we discard user-created comments, I'd rather tend towards showing copy/paste-able instructions in the console. We have a showCopyPasteInstructions and makeCodeSnippet helpers that should make this fairly user friendly.
Logaf-L though, so happy to leave the decision up to you!
| case '.json': | ||
| case '.jsonc': | ||
| try { | ||
| const config = parse(configContent) as { main?: string }; |
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.
l/m: We could try to reuse our existing parseJsonC helper and avoid adding another dependency. I'll admit, the parsing strategy is a "bit" hacky, so if there are any obvious drawbacks, feel free to continue using jsonc-parser.
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 are 3 drawbacks which I found on the quick implementation, where I'm not 100% certain if it is worth the effort to fix - since jsonc-parser does have almost 30m weekly downloads and is maintained by Microsoft.
Drawback #1. We apparently remove whitespaces
Drawback #2: Comments are sometimes moved somewhere else
Drawback #3: The complexity of the code gets a little bit more, since we have to deal with merging AST (e.g. existing array fields or existing objects), which was plain objects before see mergeValue
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.
more than enough reasons to use jsonc-parser. thanks for checking!
I removed the toml parser, since it seems really a little odd when the comments get removed. In the future we can always add it as feature in case there is demand. Cloudflare itself is also leaning more to jsonc anyways. |
|
It seems that the jsonc-parser has some problems with esbuild, as stated here. The workaround works great for the Cloudflare workers-sdk, which we can't use, since fossilize doesn't allow extra fields. I now removed the external dependencies and stick with our drawbacks, they are not that harmful. However, one test is skipped, where the comments are at the wrong place. |
(closes #1148)
This PR will update the wrangler config automagically. The wrangler config can exist in
.toml,.jsonand.jsonc(source) that is why I addedsmol-toml(BSD-3) andjsonc-parser(MIT).These are the new steps:
nameand themainline has to be changedFor now we add:
nodejs_alsinstead ofnodejs_compatto keep the bundle size smaller, and it is not yet requiredcompatibility_dateto the date of creation (suggested by the Cloudflare docs)version_metadataDrawbacks
Toml parsers are not quite popular and are not really feature complete, that is why toml files would loose all comments, once we update them (another option would be to not support(the toml parser has been removed).tomlat all and give only a hint)