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: send all workspace settings during initialisation #632

Merged
merged 2 commits into from Mar 12, 2022

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Mar 11, 2022

I discovered this when working on the testing API. Some keys were missing here, which meant that some settings weren't sent during initialisation, meaning they were stuck at their default values, even if they had been modified in the settings.json and wouldn't be changed unless the settings.json file was edited.

@kitsonk kitsonk requested a review from dsherret March 11, 2022 03:04
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Good find. I wonder if there's a way we could have a single source of truth on this (maybe read from the package.json?). Otherwise maybe we could have a test that ensures the two are in sync. Maybe not worth it though.

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 12, 2022

I wouldn't want to read them from the package.json at runtime, as that just seems like a runtime complication that isn't warranted. Of course there could be a code gen solution for it, but that also seems overkill. A test would be nice, but we currently don't have tests, which in itself is a problem, and should fix that. All that being said, they do change infrequently, so it is sort of low risk if it is remembered. I have added a warning to the shared type, which reflects what is in the package.json that we are concerned about. Hopefully that is enough of an aide-memoir when adding top level keys.

There maybe also another way to do it, just mixing in the value of deno config namespace into the object we send. This was a sort of hold-over/port of the v2 stuff that really wasn't re-visited, which I guess it should.

@kitsonk kitsonk merged commit 8ed6a99 into denoland:main Mar 12, 2022
@kitsonk kitsonk deleted the fix_workspace_settings branch March 12, 2022 08:40
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

2 participants