ListView: from 3.38 slots appear to not work #7909
Comments
|
Assuming this would not pass automated test suite, I suspect this may be caused by some version conflict on the specific project, but I can’t see where it may be coming from. |
|
My guess would be duplicate packages hanging around as described in https://github.com/adobe/react-spectrum/wiki/Frequently-Asked-Questions-(FAQs)#why-are-there-errors-after-upgrading-a-react-spectrum-package. This would cause the SlotProvider used to pass the slot specific class names to possibly have multiple versions, resulting the child components to access the wrong SlotProvider and thus not properly receive said class names from the parent ListViewItem |
|
Thanks for reacting quickly! I was suspecting some conflict of this sort, but couldn’t spot any duplicates. I made sure that among installed packages there was only one version of So I’ll have to address that first. |
|
yep that would do it, is there a particular reason you have |
|
The script was a handy find, but surprisingly after getting rid of all duplicates (as confirmed by |
To clarify, I only have However, I explicitly have to install both |
|
Eliminating all Spectrum dependencies and then adding them again was a good way to sort out all duplicates, but didn’t help address the issue. Even re-installing the same 3.37.1 results in broken behavior. I’m sure it has to do with the exact combination of packages, not just the overall version. I’ll revert back to the working combination for now. Another thing of note is that I do have to install But again, there are no duplicates among factually installed packages as per lockfile at this point, so I guess I will wait for a couple more versions to see if it resolves spontaneously. |
|
strange that fixing the dupes didn't fix the issue... Do you have a repo I can poke around in? |
I do, but probably can make it easier to investigate first with a more minimal repro… My hypothesis is a particular combination of Spectrum package versions (no duplicates) in a lockfile should cause this to happen, if it doesn’t then it’s something elsewhere. |
|
can you provide the entries you have in your lockfile for all of our packages? maybe we can spot something through that |
Non-working combinationThis is after removing & re-adding all requirements. 
Breakdowns: AriaSpectrumStatelyTypesWorking combination
AriaSpectrumStatelyTypes |
|
Here is another way to diff all changes to package versions when going from the working to non-working state, Git tree with package changes in Yarn: Complete without wildcards |
|
A very interesting development—it may, after all, be a bug(?) Based on @LFDanLu’s comment that it may be about the Running After this, the issue is no longer reproducible. |
|
Narrowed it down to exactly |
|
Manually patching back the changes in Slots.tsx don’t fix the issue, however, so it’s still unclear. |
|
Thanks for the lock file contents. I checked against our versions and every version is up to date from what I can tell. Have you checked that your build cache has been cleared? what about the package manager cache? another weird way that duplicates can happen is if you load both the common.js version and the mjs version. There's actually three different possibilities (thanks webpack 4), you could put console logs in each one at the top level to see if multiple of them are being loaded for some reason |
|
I was bouncing back and forth between the two package versions and observing the changes immediately, so caching is not the issue. @snowystinger may I ask why I am observing the following, all else equal:
Turns out, not all else was equal: only for .pnp.cjs excerpt with duplicate virtual package entries for the same package & same versionSubsequently in Yarn manifest some Spectrum packages end up depending on the first virtual copy, while others on the second virtual copy, which probably causes the issue. I was able to confirm that this weird (probably buggy, even though that bug seems fixed) Yarn behavior was triggered by the addition of
After that:
I also confirmed that the bug & the duplicate virtual package entry come back if I only add that one peer dependency back (then rebuild Spectrum & my project). |
|
Reproduction: https://github.com/riboseinc/possible-yarn-bug-repro It doesn’t seem reasonable that the same version should have multiple virtual entries, so this is either a bug in Yarn, or intended behavior of Yarn and a bug in how React Spectrum packages specify their dependencies. |
|
Apparently, in Yarn, packages with peer dependencies are instantiated once per peer dependency. Here’s a summary: https://stackoverflow.com/a/75190432. (It claims that PnP makes peer dependencies redundant. I am, in fact, using PnP in my project, and clearly it is not helping.) I suspect this may be a cause of subtle bugs on other non-trivial Spectrum-based projects. There is more in Yarn ticket. I also posted a script that detects Yarn’s virtual duplicates of this kind. The script in the FAQ recommended by @LFDanLu does not detect such duplicates. |
|
Thanks so much for the script.
It was introduced because it depends on a package below, Is there still a bug in our repo? If so, what is the high-level summary so we know what change to consider? Otherwise, can we close this issue? Thank you again for digging into this in such depth! |
|
@snowystinger Got it. IMHO this is a bug because:
If so, I see two possible (not necessarily mutually exclusive) routes to resolution:
Either route would need some more research (I am pretty sure it should be a non-issue in other package managers, but didn’t check). |
|
I will note that
is much better of a failure mode than what I am observing. That said, I believe in some package managers this will be an error because they enforce peer dependencies more strictly. I guess one option of my first suggested route to resolution above may be:
|
|
Made a PR fixing what I think needed to be addressed. Thank you so much for the reproduction repo, it was very helpful in tracking down and verifying my change. Please make sure it meets your needs as well, you have more experience with yarn pnp than I do. |
|
Thank you! Naively rebuilding things I couldn’t reproduce the fix, but I see some instructions in PR description that I failed to follow. I checked out PR’s branch, ran But there are still duplicate package entries of 
|
1 task
|
Can you run the command in this instruction and add the output here?
|
|
In meantime, I updated the reproduction repository with a minimal web app that shows the buggy layout of ListView in presence of duplicate virtual package.
Sure: |
|
So that output looks exactly the same as before my PR. I just re-ran the steps I outlined for testing in my PR and I do not get during yarn install in the test repo. Can you follow the exact steps listed in the PR for testing? Otherwise, I also ran the example repo against our |
|
Got it, will check again. FWIW Yarn’s warning does not address the virtual duplicates issue, if I understand correctly. |
I apologize, I'm not sure what you mean? I used the warning in order to determine what peers we were missing declarations on, and that seems to have addressed the virtual copies problem from what I can tell. Maybe once you've verified it on your end as well to make sure I didn't do something silly or incomplete though, so we don't conflate two things if there is more going on. That may help me figure out why I am not seeing warnings in our own local environment. Thanks for double checking me! |
I may be wrong about it, but from point 2 of yarnpkg/berry#6724 (comment) I thought that even if all peer warnings are addressed, virtual duplicate copies may be present (package managers do not guarantee single instance). |
|
I see, that is unfortunate. For this point:
I think it would be breaking, and at the very least, super annoying, as then consumers of our library would then need to provide potentially every package of our 200+ packages. That would also be terrible from a maintenance stance and when it comes to upgrading. For this point:
It'd be the same potential problem if a user used more than 1 of the mono packages. IE @adobe/react-spectrum + react-aria-components. I don't think it's reasonable to create every combination of each of these, we have 5 of them. Both of these would probably be breaking changes. Will bring it up with the rest of the team to see if anyone has other ideas. In the meantime, I think we've at least solved it in the short term by addressing the missing peer dependencies. |
|
Got it. Will have another go at reproducing the fix in the repro repository shortly… |
|
I’m just not sure why previously it was enough for me to just run It’s a bit of a mystery as to how would it see the local Spectrum without updating test repo’s package.json, but I suppose there’s some black magic. I’m using the suggested steps with the Not sure if it’s a problem or not? |
|
Sorry, verdaccio script can be a bit difficult to run if you're not used to it. It's complaining that the package was published the last time the script was run and it looks like cleanup wasn't triggered with the failure. So you'll need to manually clean up by deleting the verdaccio storage. Path should be ./storage I think. You can reference the scripts catch. Hopefully we'll also get this merged soon in which case the nightly is much easier to test with. |
|
I removed directories as per cleanup function in the verdaccio script, running it again… |
|
Still getting this output in the end: |
|
Odd, ok, well, I will try to get this merged soon, and we can test it with the nightly route instead. Thank you very much though for trying, it's appreciated. |
|
We have a nightly now, you should be able to test this. Just to make sure, I'd run it once setting all the version tags to "nightly" (this would be the fixed case) and then maybe running it again with "3.0.0-nightly-e00dc8373-250318" to make sure it actually reproduces with nightlys since the versioning is a little different (all packages are the same version number) hopefully it reproduces on the old one and not on the newest and we won't need to see what's going on with your verdaccio 🤞🏻 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Provide a general summary of the issue here
I’m not sure why, but as soon as I bump
@adobe/react-spectrumfrom 3.37.1 to 3.38 ListView slots stop working.🤔 Expected Behavior?
No change when upgrading.
😯 Current Behavior
This is how it starts to look with an
ActionButton, for example:On 3.37.1, however, it’s fine.
💁 Possible Solution
Not sure.
🔦 Context
My goal is to bump to the latest version of Spectrum, but I’m stuck on this.
🖥️ Steps to Reproduce
Use ListView in version 3.38+ with any slots (
<Text slot="description">,<ActionButton>, checkbox selection, all are broken—in DOM tree their elements simply do not get assigned their respective classes).Version
3.38.1 (I tried all of them up to 3.40.1)
What browsers are you seeing the problem on?
Other, Chrome
If other, please specify.
No response
What operating system are you using?
macOS
🧢 Your Company/Team
No response
🕷 Tracking Issue
No response
The text was updated successfully, but these errors were encountered: