show warnings in the terminal when there are non-applicable configs in wrangler config files

[dario-piotrowicz]

This PR adds warnings for when configs present in wrangler.toml/json(c) files are not applicable

Result:

I've opened this PR to address #97, but I am not sure if it fully does because my implementation consists in simply ignoring all the non-applicable configs and show an appropriate warning in such case (which, in my opinion is an appropriate behavior?). Instead in #97 @jamesopstad seemed to suggest that we should have different handling for different non-applicable settings.

However I've structured the code in such a way that modifying it to add some custom behavior for non-applicable configs should be pretty simple (so even if this doesn't fully address #97 it should be a valid starting point for that).

What do you think @jamesopstad?

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/flarelabs-net/vite-plugin-cloudflare/@flarelabs-net/vite-plugin-cloudflare@112

commit: 6f1430d

[jamesopstad]

Thanks Dario. I've added a few comments that I think need resolving.

I think this would benefit from some refactoring, renaming etc. and possibly using an approach more similar to the diagnostics in Wrangler. It's not a big deal now though and we can look at those in a follow up PR later.

[jamesopstad]

I think we should revert the changes to this file. The config used in preview is the output wrangler.json and shouldn't be validated in the same way. It also risks removing properties that we have deliberately added to the input config.

[dario-piotrowicz]

yes, you're right, honestly it slipped my mind that the preview config is the one we generate (maybe that was decided after I started working on this?)

thanks for noticing and pointing this out! 👍

[dario-piotrowicz]

No sorry, I replied to your comment too quickly, I think that the changes in this file are totally ok, as readWorkerConfig collects all the information (including the non-applicable configs) but doesn't do anything with them, it's up to the caller to the show warnings if needed (and perf wise collecting all the info is at most very neglibible).

So I don't see any issue in using readWorkerConfig here, on the contrary, I did set this in this way thinking that we should not use unstable_readConfig directly anymore but only use it thorough this readWorkerConfig utility instead (so that if the unstable function changes in the future we'll only have a single place to change, isolating the risks in our utility and making the rest of our code more resilient).

[jamesopstad]

OK. I thought that you were removing the redundant values from the config and so doing that here would cause problems. I've looked again though and it seems you're not doing that so I see your point. I do think we need to remove the values in dev and build though.

[dario-piotrowicz]

yes I was considering that, we can totally do it and keep the existing flow/logic intact, readWorkerConfig could return both the raw/original config alongside the sanitized/fixed config to use

then the caller can decide which one to use or how (or more likely we stick both on the resolvedPluginConfig object and they get picked up and used accordingly in the various places)

would that work for you?

[jamesopstad]

Sure, we can do that if you like. Personally, I don't see any problem with using unstable_readConfig directly in preview. It's arguably better because we want preview to reflect what happens when you deploy and that's the function that will be called when you run wrangler deploy.

[dario-piotrowicz]

The fact is that we've already seen unstable_readConfig break under us, so having our own wrapper around it feels very nice to me

that being said, it doesn't really add a lot of value and I wouldn't be too against using unstable_readConfig directly in preview...

I'll have a quick play with things and see what feels better 👍

[jamesopstad]

Calling them 'wrangler warnings' is a little confusing as they're not generated by wrangler.

[dario-piotrowicz]

Thanks Dario. I've added a few comments that I think need resolving.

I think this would benefit from some refactoring, renaming etc. and possibly using an approach more similar to the diagnostics in Wrangler. It's not a big deal now though and we can look at those in a follow up PR later.

Thanks 👍 , I am not too familiar with diagnostics in Wrangler, I'll have a look 👍

[dario-piotrowicz]

Thanks 👍 , I am not too familiar with diagnostics in Wrangler, I'll have a look 👍

I think you're referring to this: https://github.com/cloudflare/workers-sdk/blob/8def8c99e1f74f313a3771c827133c59e1b1032b/packages/wrangler/src/config/diagnostics.ts

Any specific reason as to why you think having something more similar to this would be better?

[jamesopstad]

Thanks 👍 , I am not too familiar with diagnostics in Wrangler, I'll have a look 👍

I think you're referring to this: https://github.com/cloudflare/workers-sdk/blob/8def8c99e1f74f313a3771c827133c59e1b1032b/packages/wrangler/src/config/diagnostics.ts

Any specific reason as to why you think having something more similar to this would be better?

I just generally think that if there's already an established pattern for something then it's worth a look. Like I said, it's not a big deal and what you've done is good.

[dario-piotrowicz]

Thanks 👍 , I am not too familiar with diagnostics in Wrangler, I'll have a look 👍

I think you're referring to this: https://github.com/cloudflare/workers-sdk/blob/8def8c99e1f74f313a3771c827133c59e1b1032b/packages/wrangler/src/config/diagnostics.ts
Any specific reason as to why you think having something more similar to this would be better?

I just generally think that if there's already an established pattern for something then it's worth a look. Like I said, it's not a big deal and what you've done is good.

ah ok, that makes sense to me 🙂, the way you put it sounded to me like you were suggesting that there were clear benefits/improvements from that approach (the only one I noticed after a quick glance was the nesting relationship there, which I think doesn't apply here).

after I address the rest of the feedback here I will have a quick play to see how that structure would translate here 👍

[dario-piotrowicz]

Diagnostics collects warnings and errors, but I think that we do want to collect more than that, we do also want to keep track of the various non-applicable fields so that we can potentially action upon them later on (if we decide to include ad-hoc handling of some non-applicable fields).

We also don't have the issue of wanting/needing to have a tree-structure, since I think that that is useful/needed when there are various environments but that in our case we only want to warn about the fields that apply to the current environment (we did discuss about this sort of thing; not to overwhelm/bother devs we said that we only want to warn about fields that are actually in use, not everything present in the config file, right?)

So I don't think that trying to copy/follow what Diagnostics does would necessarily make our code better and that it would basically boil down to change the code to have a more object oriented code structure, which I would not be opposed to (although I also don't see a huge benefit from that).

So I am again not completely sure if following wrangler's structure is necessary/beneficial here. I am happy to proceed with whatever you prefer, I don't know if you have in mind a specific refactoring (e.g. refactor things not to use getWorkerConfig in the miniflare-options.ts and have a single unique validation step for build/dev in which we have our own version of Diagnostics) if you do, would you maybe like to give your idea a try and open a PR against my branch or even a new PR against main itself so that we can compare and decide which version to go with? (I'm completely fine if we decide to close this PR and go with a different approach).

Or if it's not a huge concern we can keep the implementation as is for now, again I'm happy with whatever 🙂