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

Add validation and some small cleanup #19

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Conversation

peternandersson
Copy link
Contributor

Adds validation around passing in undefined ids, which can commonly occur when incorrectly accessing part of the plugin config. Left these validations as console.warn since we wouldn't want this to be a breaking change in case people are passing in undefined on first render, then a valid config item on the next.

Also cleans up some comments and types ahead of the next version bump.

Pearce-Ropion
Pearce-Ropion previously approved these changes Sep 20, 2024
return subscribedWorkbookVars[id];
getVariable(configId: string) {
if (configId === undefined) {
console.warn(`Invalid config variable: ${configId}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should mark these as deprecation warnings and indicate that the ability to do this will be removed in the next major version?

Suggested change
console.warn(`Invalid config variable: ${configId}`);
console.warn(`Deprecation warning: variable ID must be defined. The ability to pass an undefined ID will be removed in the next major version`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an error like this might lead new users to believe that having a defined config id is currently valid, but part of the issue with throwing here in a future version, is that when their app is not embedded into slate, it will throw an error from the config not being defined. This could be acceptable behavior, but I personally like the softer warning when the config isn't defined.

That being said, I think a good expansion to this in the future is to potentially rework the config system so that we can validate that a config value corresponds to a config option of the right type instead of just ensuring it's not undefined.

src/client/initialize.ts Outdated Show resolved Hide resolved
@peternandersson peternandersson merged commit 41cdf89 into main Sep 20, 2024
1 check passed
@peternandersson peternandersson deleted the peter/validate-ids branch September 20, 2024 23:20
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.

2 participants