fix: Allow duplicate registration of values #7924
Conversation
Also remove unused partially implemented case-sensitivity from registry.
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
maribethb
changed the title
core/registry.ts
Outdated
| * @returns A normalized string. (Ex. 'field_angle') | ||
| */ | ||
| function normalizeName(name: string): string { | ||
| // Do we need this type checking? We are using TypeScript after all. |
There was a problem hiding this comment.
While we're using TypeScript, a lot of developers aren't, so I've found that it reduces confusion when we check anyway and then issue good warnings. This only applies to developer-facing APIs, not internal-only methods. There are a few places where I wouldn't mind adding more (e.g. an FAQ in the forum is related to leaving off the workspace parameter when serializing so if we checked for that case and issued an error we could probably save ourselves some support burden)
That being said, this particular message isn't useful anymore. The original message told you it either had to be a string or had to be a Blockly.registry.Type class. So I would probably do that check in register that the name is valid, and you can eliminate it here.
| * @param opt_throwIfMissing Whether or not to throw an error if we are unable | ||
| * to find the object. False by default. | ||
| * @returns A map of objects with the given type, or null if none exists. | ||
| */ | ||
| export function getAllItems<T>( | ||
| type: string | Type<T>, | ||
| opt_cased?: boolean, | ||
| opt_cased?: any, |
There was a problem hiding this comment.
Unfortunately after looking into it I'm no longer sure this is safe to remove, at least not without doing some work to make the serializer case-insensitive.
If someone registered a serializer with an uppercase name, then their existing serialized workspace data would have uppercase characters in it. After this change, if we tried to deserialize that data, the names wouldn't match and the data wouldn't load correctly. @BeksOmega can you confirm that? Would this be safe if we changed serialization to also lowercase the state keys when loading?
I guess if someone registered both Foo and foo serializers then it wouldn't safe regardless, but I would also be deeply sad
There was a problem hiding this comment.
Yeah I believe if someone has registered a serializer with capitals (which is would be reasonable if they were using camelCase) their data will no longer load. Should be relatively easy to add a test for that.
Given that, I have a small opinion that this is not worth changing.
[Edit: missed this]
Would this be safe if we changed serialization to also lowercase the state keys when loading?
That should be safe except about what you mentioned with Foo and foo. Removing case-ful-ness is always going to have problems like that.
|
I think this PR should be against |
| * @param type The type of the plugin. (e.g. Field, Renderer) | ||
| * @param name The plugin's name. (Ex. field_angle, geras) | ||
| */ | ||
| function validateCase(type: string, name: string) { |
There was a problem hiding this comment.
I think this is now a breaking change in the other direction, we previously allowed you to ignore case (except for serializers, where the name you pass in is actually in the serialized output) so throwing an error now will break anyone who was relying on that.
There might still be some cleanup that's possible here but I don't think we should change the behavior in either direction, in other words, we live with the somewhat weird state that most registries are case-insensitive except for the serializer. And the cleanup doesn't need to block the allow duplicate registration of values since that's required for v11 so there's some urgency there, unlike the cleanup.
There was a problem hiding this comment.
The current PR will break two cases:
- Where someone is using our partial case-sensitivity to store different values for keys 'foo' and 'Foo'.
- Where someone is using our partial case-insensitivity to get away with inconsistently switching between 'foo' and 'Foo'.
I'd argue that both of these behaviours are bugs.
It would be one thing if we could just leave this mess alone, but we need to build on top of it for V11. This problem is going to contiune to compound, and we'll have a registry that behaves in random ways and is impossible to document. I think if we don't straighten the registry out now, it's just going to get worse and worse.
There was a problem hiding this comment.
I don't think it's fair to call those behaviors bugs because we explicitly built support for them (i.e. the parameter you can pass to allow for case insensitivity). In hindsight, we may have made a different decision around this if we went back in time. But now, if we want to change it, we need to do a proper cleanup which would mean weighing the value of this breaking change against the burden of communicating it to users and forcing them to do work to change their code if they're affected by it. Part of that proper cleanup would be doing a real breaking change, communicating it effectively, and not having it be mixed with the feature of allowing duplicate registration.
I hear you that we're building on top of mess, but the actual change we need for v11 doesn't have anything to do with case sensitivity and is really a very small change buried in the much larger case sensitivity refactoring part of this PR, so I don't think that's a compelling enough reason to mix the two issues.
If you want to file an issue for the case insensitivity cleanup, we can triage it separately during our normal bug triage process. I would prefer you do just the duplicate registration of values in this PR, and wait to see how prioritization works out for the rest of the cleanup vs everything else we already have planned for the v11 release
Also removes unused partially-implemented case-sensitivity from registry.
Fixes #7804