Make strongly-unique hyphen-insensitive - #704
Conversation
badeend
left a comment
badeend
left a comment
There was a problem hiding this comment.
I think this makes sense and offers wider compatibility across different languages 👍
|
Good point! Makes sense to me. It might be good to get a bit of implementation feedback (to see if this breaks anything) before merging though. |
|
Yeah, for sure. I do think this is a direction we want to go but there's no urgency to merge this; we can work around stuff for now (and are unlikely to encounter this in practice). |
|
Implementation-wise this is easy enough. I expect various minor tests to break with this change (e.g. this spec test), but I'd be surprised if this had any broader fallout. @lukewagner was there any sort of specific implementation feedback you were looking to acquire? My guess is that if this actually breaks anyone we won't actually know until late in deployment when it's practically a bit too late to roll back, but I still think that the chance of that is quite small. Basically, personally, I'd be happy to go ahead and merge the wasm-tools change into Wasmtime and such think it's reasonable to merge the spec PR here. |
|
@alexcrichton Good to hear on both counts, thanks! Then agreed to merge now. @bvisness Do you suppose you could extend the (rather meager atm) strong-uniqueness tests at the end of |
|
Yes, I can at minimum add all the new examples from the explainer, and maybe others if I think of them. |
|
@bvisness can you double-check that the tests added here are as you intended? |
|
Yeah those look good, and I suggested an extra one as well. |
|
Added. Should I just wait for a wasmtime release before merging this? I assume this is what @alexcrichton is working towards with that wasm-tools update? |
|
Personally I think it's fine to merge this whenever, we haven't historically had a strict policy about exactly when to merge and it's mostly based on vibes. For changes like this which are technically breaking but aren't expected to break anything in practice they've just been merged whenever |
lukewagner
left a comment
lukewagner
left a comment
There was a problem hiding this comment.
Since this is just a restriction (not a new feature) and implementation feedback is in, I think we can merge whenever.
) * Drop hyphens when testing equality of kebab names This commit is an implementation of WebAssembly/component-model#704 for this repository where the main change is to drop the `-` character when comparing kebab-names. More rationale can be found in the upstream PR, and while there's expected to be minor amounts of test-related fallout this is not expected to break anything in practice. * Add another test
4 participants
In #197 @lukewagner discussed making the strongly-unique algorithm also hyphen-insensitive. There was also some discussion of how strong this needed to be, but in my opinion it makes the most sense to remove hyphens entirely when checking for uniqueness, as this completely prevents names that differ only in case.
This is already a concern for us in Firefox, as an identifier like
[method]a-b.ABon resource typea-bwill produce a class namedAB(PascalCase), and a method namedAB(theoretically camelCase, but acronym). That's maybe workable for us as far as JS goes (because the constructor is just namedconstructor), but obviously other languages like Java and C# would choke on this case as well. I don't see a good reason for component interfaces to require case sensitivity.