Issue with v2 addons and ember-auto-import

[ynotdraw]

Hello! 👋

First off, thanks a bunch for this work, it's really nice! The issue I'm hitting is specific to Ember, and more specifically v2 addons.

The main issue I'm hitting is essentially https://github.com/embroider-build/ember-auto-import/issues/503.

I'm working on upgrading some v1 addons to v2. These v2 addons import fractal-page-object via ember-auto-import. After performing the migration, I saw all of our tests that were extending the PageObject begin failing. The exact error message was Custom selector()/globalSelector() class must be PageObject or PageObject subclass.

Which as you know, is thrown when isPageObjectSubclass() doesn't see the Class as a PageObject instance.

That's what lead me to the issue listed above, as well as https://github.com/embroider-build/ember-auto-import/issues/588 and https://github.com/gossi/ember-command/issues/23.

Based on those issues, it seems as though instanceof checks should not be used with v2 addons and ember-auto-import. I had originally thought that maybe we could solve this issue by adding a static member on the PageObject class; however, I don't think that'd work here after digging in a bit further.

[simonihmig]

Also worth noting: we are only talking about a temporary workaround until the underlying issue has been resolved. The alternative we already discussed was using patch-package to basically make isPageObjectSubclass always return true, but that doesn't seem ideal as well! 😅

[bendemboski]

Do we have any notion of how temporary? If it's going to be a while I can noodle on it some and come up with a workaround, and then probably later remove it. But I don't think patching isPageObjectSubclass to always return true is really that bad of an option. The only reason it exists is to validate the arguments to selector() and globalSelector() at the time they are first evaluated (so the error stack trace leads back to the field initializer in the page object class itself) rather than at the time they are accessed (in which case the error would be less explicit and would lead back to the access site, e.g. in some specific test).

So as a temporary workaround, giving up this slightly better argument validation in favor of a slightly worse one (but one that will not result in any false test positives or anything like that) seems pretty reasonable to me.

[simonihmig]

Thanks Ben for the quick feedback!

Note sure about the first part, there is an old PR (https://github.com/embroider-build/ember-auto-import/pull/512), but that doesn't seem to be a candidate to get merged. It has been identified as a priority here, but not sure what we really can expect here! 😅

As we have to get us unblocked asap, we'll probably patch the file as suggested for the time being, if that's something you think is reasonable, and a better way to fix this is not super trivial...

Thanks again!

[bendemboski]

I'm happy for some feedback/ideas here, because there's not a super obvious/trivial solution.

The two "easy" alternatives (other than temporarily disabling the check and incurring the drawbacks listed above) that I see are:

1. Some kind of duck typing
2. A WeakSet that the PageObject constructor adds its instance to, and then isPageObjectSubclass() uses .has() on the set

I suppose (1) is doable. The problem is that because PageObjects are meant to be kinda like namespaces -- intentionally allow users to subclass and add fields descriptively named whatever they want -- I try to avoid "polluting" the namespace as best as I can. So currently the only "known" properties I could use to duck type are element and elements, but that's not great duck-typing, and also if DOM Element descriptors ever land, those properties will go away as well. So I guess it would work to just put a __is_page_object__ field (or something) on the base class, and then use that to duck type...it just feels messy...but I guess if I think of it as a temporary solution then it's not too bad.

I'm not sure if (2) would be doable -- do either of you know if the same issue that causes instanceof not to work would also result in there being two copies of a module-scoped const WeakSet, such that the constructor would add its instance to a different WeakSet than the test code's isPageObject call would be calling .has() on?

I should probably add a v2 addon to this monorepo so that it's part of the test matrix -- if either if you are excited about throwing that together I'd definitely welcome a PR!

[ynotdraw]

Hey @bendemboski , I think we can actually safely close this now. Apologies for the noise.

Essentially what we uncovered was that fractal-page-object was getting imported in production code from another addon in our large codebase. The import and usage via a test helper was supposed to live in <addon>/addon-test-support/; however, it was inadvertently co-located with a component file at <addon>/components/<component-name>/test-helper.ts.

Due to our usages mostly in addon-test-support/ but having this one case, we ended up getting bit by https://github.com/embroider-build/ember-auto-import/issues/503 where we had the two versions of fractal: one in the app, another for tests, causing all of our tests to fail due to what we described above and also due to a WeakMap that was getting instantiated twice in page-object-state.

The fix for us was to properly move the test helper file from <addon>/components/<component-name>/test-helper.ts to <addon>/addon-test-support/components/<component-name>/test-helper.ts instead. After doing this, all of our tests went green.

Once again, I apologize for the noise. It took us a few days to get to the bottom of it.

[bendemboski]

No problem, glad you got it sorted out!