Enable page object composition via stored definitions

[mistahenry]

Updated description

Edit: Originally this PR was an attempt to solve both composition and extension of page objects. During the course of review, the extension aspect was removed. While it no longer solves #181 , the tools are in place for an extension approach via some sort of custom merge function should someone wish to write that in app space (or should such an approach be vetted and accepted here).

Leveraging Ceibo's ability to take custom buildObject functions, the definitions for each page object are stored in the Ceibo metadata so that that during composition, the page object can be substituted with its definition. This is necessary since Page object's are "special" in that they fire functions expecting a test context and make assertions about the number of elements they select when accessed. Definition storing is preferable to a reflection-based approach in which we would extract the property descriptors and clone the getters since this can easily lead to closure scoping bugs without a great deal of care. See this comment for an example of this type of hard to track bug.

Now, the following syntax is possible:

let screenPage = create({
  value: value('.screen input'),
  fillValue: fillable('.screen input')
});
let page = create({
  visit: visitable('/calculator'),
  screen: screenPage  
});

Collections can also now take a page object as the definition directly

test('collection supports taking a page object directly as its definition', async function(assert){
   const textPage = create({
     spanText: text("span")
   });
   let page = create({
     scope: '.container',
     collection: collection("li", textPage)
   });
    await this.adapter.createTemplate(this, page, `
     <ul class="container">
       <li>Text <span>Lorem</span></li>
       <li>Text <span>Ipsum</span><>li<
     <ul>
   `);
    assert.equal(page.collection.objectAt(0).spanText, 'Lorem');
    assert.equal(page.collection.objectAt(1).spanText, 'Ipsum');
  });

Besides unlocking the possibility to merge page objects (while accessing private members at ones own risk), which would allow users to only store page objects should they so wish, this PR removes a pitfall of page objects that I'm sure I wasn't the only one to run into.

Original Description

(since a lot of the subsequent conversation refers to these notions): This PR solves #181 and enables page objects to be used directly within definitions as well as exposes an extend function to be used when creating new page objects from existing ones. The general idea was to store a sanitized version of the page object's definition within the page object itself in its Ceibo metadata. Sanitized in that if the definition contains page objects, it will replace the child page object with said object's definition before storing.

I'm happy to add documentation but wanted this approach to be finalized first.

An example of what's valid now:

let keyboard = create({
  scope: '.keyboard',
  numbers: collection(".numbers button", {
    text: text(),
    click: clickable()
  })
});
let screenPage = create({
  value: value('.screen input'),
  fillValue: fillable('.screen input')
});
let page = create({
  visit: visitable('/calculator'),
  //extension
  keys: keyboard.extend({
    clickOn: clickOnText('.numbers'),
    sum: clickable('button', { scope: '.operators', at: 0 }),
    equal: clickable('button', { scope: '.operators', at: 2 })
  }),
   //composition
  screen: screenPage  
});

[mistahenry]

A side effect of this PR solves a bug in the current implementation which prevents deep merging when collections are present.

A test as simple as this will fail:

test('returns an item', async function(assert) {
    let definition = {
      foo: collection('span', {
        text: text()
      })
    };
    let extendedDefinition = $.extend(true, {}, definition);
    let page = create(extendedDefinition);

    await this.adapter.createTemplate(this, page, `
      <span>Lorem</span>
      <span>Ipsum</span>
    `);

    assert.equal(page.foo.objectAt(0).text, 'Lorem');
});

with the error message because of the assertion:

TypeError: Cannot read property 'call' of undefined
at Object.foo ... /ceibo/index.js:79:1)

refering to this block of Ceibo code:

function buildDescriptor(node, blueprintKey, descriptor /*, descriptorBuilder*/) {
  if (typeof descriptor.setup === 'function') {
	descriptor.setup(node, blueprintKey);
  }
  if (descriptor.value) {
	defineProperty(node, blueprintKey, descriptor.value);
  } else {
	defineProperty(node, blueprintKey, undefined, function() {
	  return descriptor.get.call(this, blueprintKey);
	});
  }
}

The else path is being taken because the passed descriptor doesn't have a value. The descriptor in question is the return value of the collection function (main.js not legacy) invoked when creating a page object's definition:

export function collection(scope, definition) {
  let descriptor = {
    isDescriptor: true,

    setup(node, key) {
      // Set the value on the descriptor so that it will be picked up and applied by Ceibo.
      // This does mutate the descriptor, but because `setup` is always called before the
      // value is assigned we are guaranteed to get a new, unique Collection instance each time.
      descriptor.value = new Collection(scope, definition, node, key);

      if (window.Proxy) {
        descriptor.value = proxyIt(descriptor.value);
      }
    }
  };

  return descriptor;
}

As you can see, this setup function is supposed to create the the value (and it does when invoked by Ceibo during creation of the tree). The problem here is a nasty little closure scoping bug because when we deep merge the original definition via $.extend(true, ...), we are sharing the same reference to the setup function and its entire closure scope, but are returning a new descriptor. In code terms:

definition.foo !== extendedDefinition.foo
definition.foo.setup === extendedDefinition.foo.setup
//imagining you could access the closure scope of a function directly
extendedDefinition.foo.setup.closureScope.descriptor === definition.foo

So when we invoke the setup function of this new descriptor (extendedDefinition.foo), we are adding the value property to the original descriptor instead of the extended.

I've fixed that here by always recreating collections within a call to create to avoid the sharing of state altogether.

[coveralls]

Coverage increased (+0.05%) to 98.537% when pulling 216ae40 on mistahenry:enhancement/composition-and-inheritance into ce5deab on san650:master.

[ro0gr]

First of all, thank you for taking your time to explore this area!

I think there are actually 2 things you are trying to address in the PR:

• Fixing create behavior when dealing with instantiated page objects
• Introducing new extend API

I'd like to keep it in separate PRs in order to better understand scope of the changes.

However, let me share my general thoughts regarding these topics.

create

It took me some time to get used to live with a single top level create in my codebases. I just forgot the issue exists at all 😆 That's why persisting of original definition in order to recreate page objects on demand made me feel uncomfortable for a moment. However I realize, it may be the only straightforward approach to the issue.

There are few observations regarding the current implementation:

• Since the need to store definitions comes from getter properties it might make sense store only them rather than the whole definition.
• As far as I understand we currently only store definition at the top level page object passed to create.

I suspect the following scenario can break:

const a = create({
  b: {
    scope: '.b'
  }
});

create({
  b: a.b
})

If that's true we should probably integrate a definition persistence trick with ceibo builder methods(buildObject, buildChainObject) to be recursion aware.

extend

Being honest, I don't think this is a good feature to have(but I can be wrong!).

If think in classes, I associate definitions with classes and page objects with objects created from a class with a new operator:

class A {} // or even EmberObject.extend({})
const a = new A(); // or A.create();
a.extend() // ???

I think extend on JS object instances doesn't make sense. But that might be just a naming issue(merge, override,... as a possible alternative?).

I don't actually know how does $.extend work, but assuming it does a deep merge, I'm in doubt if we really want a deep merge. It just feels too complex to me. Again, if thinking in classes deepMerge even doesn't have an analogy.

I'll think about it more and comment in the ticket you've linked to in the PR.

[ro0gr]

is this only needed because of setup issue you've mentioned in the comment #430 (comment) ?
If so, I think we might have a possible fix for that #406

[mistahenry]

Yes, I just validated that if this PR was merged, I would no longer need to reinitialize all collections. Anything in particular holding this up from merging?

[ro0gr]

Thanks for confirming!

Well, my original intention to keep it un-merged was not to pollute v1.15(which is already quite full) with non-critical, but potentially harmful refactorings Unfortunatelly cutting a stable release of v1.15 hanged longer than I expected.

I think since there are no any user reports about any issues related to the current collection setup we can leave it un-merged some more time...

Meanwhile I think you can cherry-pick it to your branch to avoid the issue you have faced.

[ro0gr]

I think in a long run we aim to get rid of jquery. I'd like not to introduce new jquery usages if possible

[mistahenry]

I could rely on assign for shallow merging. For deep merge without jquery, an out of the box solution that's tiny is deepmerge. Alternatively, hand rolling my own isn't that complex, it just requires specially handling of what object types are mergeable and a choice on how to handle arrays

[mistahenry]

I think extend on JS object instances doesn't make sense. But that might be just a naming issue(merge, override,... as a possible alternative?).

I thought about using copy/clone with overrides behaving like extend, but one would expect this to work without a subsequent call to create from the semantics of copy/clone. Consider this scenario:

let screenPage = create({
  value: value('.screen input'),
  fillValue: fillable('.screen input')
});
let page = create({
  //composition
  screen: screenPage
});

Would a user think it was necessary to call screenPage.clone() when composing? Technically it wouldn't be necessary, I just wonder if it's confusing?

$.extend({}, someObject, { /* overrides */ } or similarly with _.extend is a pretty common idiom for creating a new object from an existing one. I do see your point that it may be confusing since you're not technically extending the object but instead its hidden defintion. But the call to create at least makes it somewhat clear that a new object is being created.

What @san650 suggests here is what I have used in a number of projects. The approach certainly works but feels like a workaround at best. I considered originally something along the lines of create(pageObject.toDefinition({ /* overrides */ }) but found it requires unnecessary mental overhead, in that it requires developers to make the distinction between page objects and page object's definitions (which is large part of my objection to the current $.extend({}, definition) approach to begin with). The whole idea of separating the definition from the page object is cumbersome, unintuitive for new users, and ultimately more verbose than necessary.

Being honest, I don't think this is a good feature to have(but I can be wrong!).

Look at my example. In such an example, being able to insert/override certain properties (ie deep merge) is crucial to avoid repeating myself averywhere. Ie, without the facility to deep merge, the extend function would be of little value in such a scenario. Furthermore, the ability to overide scope selectors of component page objects with a more specific selector when used within acceptance tests is always necessary for me when two of the same type of component are siblings (ie, always in forms). I'm talking about:

<div class="form-element-container">
	...
</div>

let inputComponentPage = create({
	scope: ".form-element-container",
	...
})

let acceptanceTestPage = create({
	username: inputComponentPage.extend({
   		scope: "[data-test='username-input']"
	}
})

where I target a specific instance of the inputComponent use a data-selector in my route's template.

I realized now, though, that the extend function should take the deep merge as an option in the same way that $.extend does some scenarios truly call for replacing a child object rather than deep merging.

[mistahenry]

I'd like to keep it in separate PRs in order to better understand scope of the changes.

I cannot realistically open both simultaneously since both of these PR's require a mechanism for navigating a page object without firing the getters. If you feel it necessary, I can redact the extend code and open it once we sort out the composition.

I chose storing the definition since I didn't see any drawbacks and it seemed easiest. To your point

const a = create({
  b: {
    scope: '.b'
  }
});

create({
  b: a.b
})

wouldn't work due to the dsl that's applied to all page object nodes. My solution will have trouble here when trying to enumerate the dsl created getter properties like value, but that's neither here nor there since Ceibo would also invoke the getter in processNode when recursively creating the tree.

Now that I think about it, storing the definition may not even be necessary. Since all page object's have a root node that isn't getter based, a recursive descent using Object.keys + Object.getOwnPropertyDescriptor + Object.defineProperty would allow create to properly sanitize any page objects and getters within the passed definition object. But at that point, we'd be sharing functions (since i don't see how you get around copying the get function without storing the definition), so seriously special care must be taken to avoid subtle closure scope sharing issues like the one I experienced with collection. That was seriously cryptic to debug.

If that's true we should probably integrate a definition persistence trick with Ceibo builder methods(buildObject, buildChainObject) to be recursion aware.

If we did store the definition per property, we'd avoid the sharing of the getters mentioned above, but could still use said sanitization approach. But that's really only enabling sharing a subtree of the page object, which more than likely would have been represented by a page object to begin with?

[ro0gr]

I've just commented in the ticket with my objections to deep merge and shared an alternative approach to the problem. I believe it should address most of the concerns and might mitigate the immediate need for extend.

The whole idea of separating the definition from the page object is cumbersome, unintuitive for new users, and ultimately more verbose than necessary.

I actually love having defs as POJOs 😊 It's very simple and flexible to me.
Though I agree it may be unintuite for new users from the first glance. I believe we exploring of class usage for definitions may improve learnability.

[mistahenry]

Are you and @san650 and the rest of the maintainers opposed to the idea of composition altogether?

As far as I can tell, this PR is fundamentally exactly the same approach as storing / exporting both the definition and the page objects, but absolves the user from that kind of overheard. You can continue to use your pojo composition approach, while anyone who wants just page objects can compose them in a way they would have always expected to work out of the box. Supporting page objects within definitions seems like a clear win for everybody.

And maybe the value of extend isn't clear. Without composition, there is no point. But when one starts using page objects in definitions, we need an Object.assign that performs the necessary page object -> definition substitution. I'll capitulate on the deep merge because with composition + shallow merging, I am capable of achieving my goals in a way that feels idiomatic. Whether it's somePageObject.extend, or clone(somePageObject, { /* overrides */ } is simply a design choice. Or even an alternative constructor?

export function merge(){
  let sanitizedArgs = ([...arguments]).map((opt) => { 
    if(isPageObject(opt)){
       return assign({}, Ceibo.meta(opts).pageObjectDefinition);
    }else{
      return convertPageObjectPropsToDefinitions(assign({}, opt)
    }
  })
  return create(Ember.assign({}, ...sanitizedArgs);
}

is the best approach?

Assuming we can move forward this, I will:

1. Cherry pick your commit from 406
2. Remove my recreateCollections code and all special handling in the collection
3. Convert extendPageObject -> assign instead of $.extend(true) effectively shallow merging

[ro0gr]

Are you and @san650 and the rest of the maintainers opposed to the idea of composition altogether?

Of course no. I believe we just have some disagreement on a topic of page object instances extensibility.

Supporting page objects within definitions seems like a clear win for everybody.

Agree! I think the fact that we currently can't pass a page object instance to the create is not a good thing. I'd be glad to see it fixed.

And maybe the value of extend isn't clear.

TBH that's still true to me. I think I understand your need of a built-in capability for extension, however I still think page object instance is not a good place to be extended/merged. I'd prefer to keep this feature separate from the create issue. This way we could keep better focus on changes and their design.

1. Cherry pick your commit from 406
2. Remove my recreateCollections code and all special handling in the collection
3. Convert extendPageObject -> assign instead of $.extend(true) effectively shallow merging

I'd like to keep the 3rd bullet point separate. Maybe it's even worth to create a ticket for the new proposed design and use cases. I think I just need some more time to think about it...

[mistahenry]

Are you and @san650 and the rest of the maintainers opposed to the idea of composition altogether?

That probably didn't come out like intended (ie I wasn't being snarky). I meant against composing page objects, as in supporting them within definitions. I ran across this closed PR this morning which made me concerned that this feature wasn't wanted at all. I'll do 1 and 2 and remove the extend for now.

[ro0gr]

Let's not make it public?

[mistahenry]

fixed

[ro0gr]

I think this is a bug. Since we are going to fix create for page object instances it should be fixed for any level of nesting:

const { b } = create({
  b: {
    scope: '.b'
  }
});

const c = create({ b })

I believe b is also a page object instance, the same as a b's parent is.

[mistahenry]

fixed

[ro0gr]

I would actually prefer to have a specialized methods for getting and setting of pageObjectDefinition like we do for isPageObject. Also can we make it harder to access it in the user land by adding a bazzilion of underscrores in the name __pageObjectDefinition__ or maybe even using a Symbol(not sure if the later is applicable here).

[mistahenry]

yeah good idea. fixed

[ro0gr]

I don't know exactly how to do it yet, but... but I think we really need to do it in custom ceibo builders land in order to recursively store definitions per instance. Doing it in create only partially solves the issue

[ro0gr]

wow.. seems like I have something working:

function buildObject(node, blueprintKey, blueprint, defaultBuilder) {
  // here we check if we are going to create against the page object instance
  // in case if that's the case use its definition
  let definition;
  if (Ceibo.meta(blueprint)) {
    definition = Ceibo.meta(blueprint).__poDef__;
  } else {
    definition = blueprint;
  }

  blueprint = assign({}, dsl, definition);

  const [ instance, blueprintToApply ] = defaultBuilder(node, blueprintKey, blueprint, defaultBuilder);

  // persist definiion once we have an instance
  Ceibo.meta(instance).__poDef__ = definition;
  
  return [ instance, blueprintToApply ];
}

I've replicated the same trick to the buildChainObject.
I've just hacked it in node modules of my one tiny project and didn't test it seriously. But at the first glance seems like it works..

What do you think? can we use it instead create-only?

[mistahenry]

Definitely the right track, which I've tailored into a working implementation. Above will fail here:

let definition = {
  foo: {
      baz: "prop"
  }
};
let page = create(definition);
let pageComposer = create({
  somePage: page
});

because the initial blueprint isn't a page object, which means that blueprintToApply will contain the page object and will fire off getters in the next processNode when buildObject is called again and blueprint = assign({}, blueprint); will fire off a getter as soon as it encounters somePage.

That can be refactored away, but even still, there's a subtler problem. The first invocation of buildObject on

{
  somePage: page
}

will lead to an definition still containing page objects to be stored since the definition object isn't currently a page object. Any subsequent creation will would fail to compose this page object properly since the stored definition must be a pojo.

I find it much more straightforward to always sanitize the definitions as soon as possible such that we are dealing with pojos once in Ceibo.create. It's also more performant than adding those additional checks with then buildObject itself.

[mistahenry]

For the record, this latest build failure is a result of ember-cli-addon-tests 199

[ro0gr]

Above will fail here:

let definition = {
foo: {
baz: "prop"
}
};
let page = create(definition);
let pageComposer = create({
somePage: page
});

Sorry, I can't reproduce a failure you've described. PO seems to work fine w/o any create issues on my end.

[ro0gr]

I think we should avoid introducing of one more recursive mechanism. We already have ceibo for that.

[mistahenry]

I've replicated the same trick to the buildChainObject.

I was originally confused why you'd done anything here.

function buildChainObject(node, blueprintKey, blueprint, defaultBuilder) {
  blueprint = assign({}, blueprint);

  return buildObject(node, blueprintKey, blueprint, defaultBuilder);
}

This seemed like harmless delegation, so I only used your code in buildObject . But when looking again, it was the assign in this function that was actually the source of all the problems I was still experiencing with your approach.

Two remaining points then:

1. I am going to remove the buildChainObject and use buildObject for both since as far as I can tell from here, it's an artifact of approach that was ultimately overhauled into a noop.
2. I think I should avoid storing the _chainedTree on the top level page object unless you can think of a solid reason to since it would be unnecessary work.

[ro0gr]

Just left few comments. At this point I need some time to get confidence about what we are doing with a _chainedTree. Going to play with it this weekend.

I think we don't have to test "re-creation" for all the existing attributes. I'd suggest to test just few of them:

• some action
• some getter
• collection
• probably alias, as

Also I think it makes sense to group such tests in a single(or maybe few?) file, just to have a better perspective of the feature test coverage.

[ro0gr]

we can revert to just re-exporting it.

[ro0gr]

Can we use getPageObjectDefinition(page) instead of Ceibo.meta(page).__poDef__?

[ro0gr]

not sure what is the intention here. The pageComposerDef.somePage is manually set to pageDef a line above. So how is this assert different from simply comparing pageDef and definition?

assert.deepEqual(pageDef, definition);

[mistahenry]

you're right. that was actually not the best way to test it (nor was it really testing anything). My updated test should be clearer.

my intent was to more or less manually do what ceibo does recursively just to further prove that the definitions are stored correctly and that substitution produces the expected definition obj (as if only definition pojos were merged in the first place)

[ro0gr]

This replacement trick still feels dirty to me. I can't see how it's better than simply:

assert.deepEqual(somePageStoredDef, definition);

prove that the definitions are stored correctly and that substitution produces the expected definition obj

Do you want to make sure that there is only somePage defined inside of pageComposerDef? if such we could additionally check for all the existing keys:

assert.deepEqual(Object.keys(pageComposerDef), ['somePage']);

[ro0gr]

Just wondering how does eslint ignore multiple empty lines. Probably something wrong with its setup 🤔

[mistahenry]

At this point I need some time to get confidence about what we are doing with a _chainedTree.

@ro0gr Understandable. I reasoned the removal of the separate chainedObjectBuilder seeing as:
a). it looked like a simple passthrough to buildObject
b). there was evidence in the PR that introduced it that it was used during the initial implementation but was later changed to no longer be different from the buildObject that's now used before ever merging
c). all tests succeeded when removing

My logic for not storing the _chainedTree:
a). all page objects must pass through create which will always create the _chainedTree node
b). storing the _chainedBuilder descriptor could have unforeseen side effects later on
c). the _chainedTree is always explicitly checked on the root node, which receives the _chainedTree during create.

I may have missed something, just wanted to pass on my thinking

[ro0gr]

Just left some comments.

I've verified _chainedTree behavior and I'm glad we've got rid of that unused/confusing part 👍

Also I've detected an issue with passing a page object as a root of create:

const a = create({ 
  // ... 
})

const b = create(a); // <= this fails 

I think, in order to be a complete solution, this use-case should be handled as well. What do you think?

[ro0gr]

I think if we ever reach this line - we have a bug. I'd prefer to change this to throw an exception, in order to detect possible issues faster.

[ro0gr]

This replacement trick still feels dirty to me. I can't see how it's better than simply:

assert.deepEqual(somePageStoredDef, definition);

prove that the definitions are stored correctly and that substitution produces the expected definition obj

Do you want to make sure that there is only somePage defined inside of pageComposerDef? if such we could additionally check for all the existing keys:

assert.deepEqual(Object.keys(pageComposerDef), ['somePage']);

[mistahenry]

@ro0gr I added support for page objects directly as the definition to create. What would be the use case?

[ro0gr]

@mistahenry I don't think there is an immediate use case for that. It just doesn't feel good to me to not handle the use case. As an alternative solution we could throw in case of re-creation dirrectly from the PO instance, but what would be the reason to fail?

I think POs re-creation can be helpful for #433. With your last fix for direct page object re-creation it may be possible to avoid introducing new APIs like fork and use create instead(just a raw thought).

[ro0gr]

context seems unused

[mistahenry]

page.setContext(context); at the end of create.

[ro0gr]

Sure, sorry, missed that 🙈

[ro0gr]

Would you mind to squash commits into one please? There is some strange diff in the collections which seems obsolete. I guess squashing should fix it.

[ro0gr]

@mistahenry PR looks good to me. Going to test it against my current app tomorrow(haven't had a chance today) and merge it in case of no issues detected. I'm pretty sure it will be fine!

Meanwhile, I think it's worth to update PR description to exclude extend notions, just to avoid possible future travelers confusion. Also it would be brilliant if you can add yourself to the AUTHORS at the root of the project in scope of the PR.

Thanks for taking your time!

[ro0gr]

Just verified it and found an issue(#406 (comment)) related to my recent collection change. Not sure if we depend on that change here anymore. Will revert it soon and verify again.

[mistahenry]

@ro0gr the change for using a getter instead of the setup function for the new collections is in here

[ro0gr]

Just merged #440, please update your branch and let's see how to make it work

[mistahenry]

@ro0gr No change necessary. I only had problems with the the collection's setup descriptor manipulation approach when trying to deep merge page objects into a new object via extend (causing the copied setup to still have closed over the original descriptor).

[ro0gr]

Thank you!

[ro0gr]

going to publish it as a patch version soon. @mistahenry @san650 objections?

[san650]

@ro0gr sounds good

[ro0gr]

published as v1.15.1