Ensure a valid target when this.only is not available

[taras]

Purpose

This PR addresses an issue discovered while getting BigTest to work with Angular. At this point, it's not clear if it's Angular/Webpack specific or a bug in Interactor. The problem occurs when you have interactor:

@interactor
class AppInteractor {
  static defaultScope = 'app-root';

  message = scoped('[data-test-message]', {
    text: text()
  });
}
let app = new AppInteractor();

Trying to access app.message.isPresent or app.message.text causes an exception saying that this.only is not a function.

Method

I'm only calling this.only is this.only is present, otherwise use this as target for the interactor invocation.

TODO

• Figure out how reproduce it in tests (it'll require typescript setup)

[cowboyd]

I wonder if this is a difference in the way the typescript compiler and babel interpret decorators. What happens if you use it not as a decorator, but as a function? Also, might be worth compiling with babel and with typescript and seeing what the output JS looks like.

[wwilsman]

Strange 🤔

That chainable function only gets used down below where only is added during the same exact defineProperties operation.

interactor/src/utils/parent-chainable.js

Lines 115 to 116 in b228141

	// provide method for breaking the parent chain
	only: {

So if it's hitting chainable it should have only unless the context is being swapped out somewhere else, which should be fixed there instead of here.

Regardless, the logic in this file is going to change slightly in an upcoming release so hopefully that will fix it. If you can reproduce it and add some tests, I'll make sure those tests are covered with the new changes before those get released.

[wwilsman]

Oh, and an aside: you don't need text: text() as text is already a default property.

https://www.bigtestjs.io/guides/interactors/available-interactions/#default-interactions

[taras]

What happens if you use it not as a decorator, but as a function?

Same result - doesn't work in the same way.

I wonder if this is a difference in the way the typescript compiler and babel interpret decorators.

The dist files for @bigtest/interactor are compiled by the rollup configuration in @bigtest/interactor package. So there should be no difference in compiled output wether consumed by TypeScript or anything else.

Also, might be worth compiling with babel and with typescript and seeing what the output JS looks like.

I don't think it's related because @bigtest/interactor is not compiled by TypeScript.

Regardless, the logic in this file is going to change slightly in an upcoming release so hopefully that will fix it. If you can reproduce it and add some tests, I'll make sure those tests are covered with the new changes before those get released.

This is blocking us on being able to use scoped in Angular, but I'm also really not sure how to reproduce it :( Not sure what to do.

[wwilsman]

If this is specific to angular and can't be reproduced in vanilla JS, then is it an interactor issue or an angular issue? We might be able to work around it if we can reproduce it.

If angular is changing the context of methods for some reason, then I'd expect more bugs besides this one. I'm not sure checking for the existence of only is a valid solution since that method should never be entered anyway if only doesn't exist.

[wwilsman]

What happens if you use it not as a decorator, but as a function?

Are you guys using the latest decorator stage-2 proposal? The old stage-1 proposal changed drastically so using the decorator as a function is no longer supported. Support is still in the current version of interactor for migrating from the old syntax but will be deprecated and eventually removed in the 1.0 release.

[wwilsman]

Maybe as a workaround try using Interactor.from(object) instead of the class syntax?

[taras]

I'm trying to figure out how to verify this, but it seems that TypeScript only supports legacy decorators. Support for ES decorators is on the roadmap for the future. For TypeScript, we might need a special work around.

[taras]

Ok, I found something weird that might give us a hit about what's going on.

It looks like this problem happens when this.$root is ready, but it's getter is actually wrapped in chainable. Is $root supposed to be wrapped in chainable?

[wwilsman]

I could see how that might cause some weird behavior. Again though, nothing should be getting wrapped at all unless there is also a this.only method since it all happens in the same defineProperties call. Also weird that this only happens in Angular and not anywhere else.

The logic around the root element is one of the things being changed in the upcoming release, so hopefully that will solve this issue if it is the problem. In the meantime you could add $root to this line:

interactor/src/utils/parent-chainable.js

Lines 94 to 96 in b228141

	// do not include the constructor or non-configurable descriptors
	if (key === 'constructor' ||
	descriptor.configurable === false) {

And now that I'm looking at that line does Angular / TS do anything that would default to configurable: true? The official spec defaults to configurable: false for defineProperties. So if that is different it might explain why $root is being wrapped when it shouldn't.

[taras]

And now that I'm looking at that line does Angular / TS do anything that would default to configurable: true?

There is definitely some funkiness in TypeScript world around configurable: true as you can see in microsoft/TypeScript#3610

I haven't been able to find the connection with our $root but it's definitely configurable: true.

[wwilsman]

Wow. Opened 3 years ago and still not fixed. Looks like targeting es6 instead of es5 might make it default correctly?

[taras]

I can't seem to figure out what causes $root to become configurable:true but it's definitely happening in TypeScript even with es6 as a target. I'll go ahead and add $root to the conditional above.

[taras]

@wwilsman do you have objections to merging this as is?

[wwilsman]

I'd like to somehow get some tests around this since I just recently got another branch to 100% coverage. But seeing as it's a bug in typescript, idk how we'd test it

[taras]

@cowboyd any idea how we could test this scenario?

[wwilsman]

It's similar enough to another change being made, so it probably isn't necessary to test since it will change anyway.

Just add something to the changelog so the bot is happy and I'll approve 👍

[taras]

Cool, I'll do it later today.

[taras]

I added it :)

[wwilsman]

Sweet!

Last thing, if you wanna release this immediately just increment the version number and update the changelog again. That way we don't have to open another PR for release (everything should be tagged automatically for us after merging)

[taras]

It should be good to go.

[wwilsman]

Released!

[taras]

Whoop whoop - BigTest in Angular time!