ERM-2395: Remove @folio/stripes-testing as direct dependency of stripes-erm-components #563
Conversation
…erm-components ERM-2395
|
Kudos, SonarCloud Quality Gate passed!
|
…erm-components (#563) ERM-2395
|
Hey @EthanFreestone, removing In
If I understand correctly - we are only installing I don't understand, however, why Jenkins reference builds are not failing... perhaps it's installed from some other module |
|
@BogdanDenis I don't have good context here. Which "Jenkins reference builds" are you referring to? IIUC, when building any module in CI, we simply run Since there are no tests defined at the platform level, Possibly additionally, (total shot in the dark) |
|
@zburke Perhaps you're right, that this issue should be fixed in this module, since it exports interactors and doesn't depend on @EthanFreestone would it be ok to revert this PR? |
|
I can revert and then re-release, but does that not still leave us with dependencies out of whack, since stripes-testing would remain a full fat dependency in this module? What's needed for Nolana first and foremost, and then what can I do to improve this situation moving forward? One idea I've been toying with is to have a devDep on a new module, stripes-erm-testing, which we could then export these kinds of interactors/mocks from. Won't get into Nolana ofc, but could help for Orchid and beyond. I'm loath to just revert this before we know what needs to happen in Nolana, because that would leave a release out of sync |
|
Such a module wouldn't necessarily even need to be tied into FOLIO releases mind, similar to what we do with |
|
@EthanFreestone, we also need to remove |
|
@EthanFreestone, oooof, I'm feeling slow and just now realizing the full scope of the issue here. I apologize for not reading your comments more carefully earlier. I appreciate you trying to do The Right Thing by formally exporting things designed to be shared into other repos and apologize, again, if this this feels like a "no good deed goes unpunished" situation. This situation is double-edged: We have a long, dirty history of allowing ui-apps to reach directly into the private paths of stripes-core and stripes-components to gain access to shared testing infrastructure. Eww. OTOH, test infrastructure is just that, and it definitely shouldn't be included in a package's exports. It'll get pruned if tree-shaking is properly configured when building the production bundle (which appears to be the case here 😌) but that doesn't feel very clean either. My inclination is the same as yours: either provide an alternative repo like In the short term, my inclination is to remove the test-infrastructure exports here and move them into stripes-testing. Alternatively/temporarily we could let other packages' tests import them directly via private paths, though I'd really rather not perpetuate that gross practice. What do you think? |
|
It's a tricky one. We have on our docket the cleaning up of our whole testing situation. I hate the current imports we're making in each test, and would like to make proper use of jest's manual mocking features (We have examples for this already in However that doesn't really solve the issue of our main point of contact for testing being a library we need a direct dep on. Does this need to be fixed in a Nolana release? If so then this would require a re-release of all of our modules, either pulling in the new exports from stripes-testing or from stripes-erm-testing... This is already one of our top priorities for Orchid. Has something changed significantly that we can't revert this PR for Nolana and then work on fixing all these imports for then? |
Revert "ERM-2395: Remove @folio/stripes-testing as direct dependency of stripes-erm-components" ERM-2395/PR #563 solved a small problem by removing some test-related dependencies which have unsatisfied peer-dep warnings when built at the platform-level, but doing that created a much worse problem that actually resulted in a build failure at the platform level because some public test infrastructure relied on those dependencies. It's no good having a noisy platform build, but a having a broken platform build is (of course) even worse. And since treeshaking is doing its job for us and keeping those test-related deps out of the final bundle, it's clear the prudent thing is to just go back to how things were pre-563 and publish yet another patch release. Sorry for the churn.
|
@BogdanDenis this PR has been reverted. Please LMK if you're still having build problems in the Rancher envs that seem related. |
No description provided.