Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): chainable PageElements #1864

Merged
merged 6 commits into from Aug 23, 2023
Merged

Conversation

jan-molak
Copy link
Member

@jan-molak jan-molak commented Aug 21, 2023

This change enables PageElements to offer more advanced chaining patterns
and DOES NOT require any modifications to the existing client code.

PageElements now allow for defining the container element at runtime, separately from defining the filtering chain.

For example:

const shoppingList = () =>
  PageElement.located(By.id('shopping-list'))
    .describedAs('shopping list');

await actor.attemptsTo(
  Ensure.that(
    Text.of(
      green(items())
        .first()
        .of(shoppingList())
    ),
    equals('lettuce'),
  )
)

PageElements now also allows for using pre-composed filter chains, like the one above, in .where clauses:

const shoppingLists = () =>
  PageElement.located(By.css('.shopping-list'))
    .describedAs('shopping list');
    
// find the last of container items that contain a specific first child item
shoppingLists()
  .where(Text.of(green(items()).first(), equals('lettuce'))
  .last()

To enable advanced filtering patters, `PageElements.located(..)` call will no longer return a
`PageElements` object. Instead, it now returns a `MetaList\<PageElement\>`, which still resolves to
`PageElement[]`, but allows for additionally allows for chaining using the `.of(parent)` call. This
change should not require any changes to client code. However, any custom questions, interactions or
tasks that relied on receiving a parameter of type `PageElements` should expect a
`MetaList\<PageElement\>` instead.
@jan-molak
Copy link
Member Author

Hey @viper3400, I'm expanding PEQL to allow for a new query pattern. Would you mind taking a look?

@jan-molak jan-molak added @serenity-js/core @serenity-js/web Screenplay Pattern APIs for interacting with the Web labels Aug 21, 2023
@jan-molak jan-molak changed the title feat(web): replaced PageElements with a generic MetaList<PageElement> feat(web): replaced PageElements with a more flexible MetaList<PageElement> Aug 21, 2023
@jan-molak jan-molak changed the title feat(web): replaced PageElements with a more flexible MetaList<PageElement> feat(web): replaced PageElements with MetaList<PageElement> Aug 22, 2023
@viper3400
Copy link
Sponsor Collaborator

Hi @jan-molak

I'm currently in holidays so let me give you just some initial feedback without diving to deep into the code.

First, I always liked the idea of this "smart chaining" in Serenity/JS.

But I always felt a bit uncomfortable with the introduction of this Metathing, as well. I know what a question is, I know what a list is - but a MetaQuestionand a MetaList - needs some more explanation. For me it sounds like something great, such as MegaList, SuperList, HyperQuestion ;). But at the end this are just object names, right?

Explanation of https://serenity-js.org/api/core/interface/MetaQuestion/ always sounded a bit stiff to me.

Okay, back to this PR.

What comes into my mind: We should have some clear examples what the goal and the possibilities of this chaining is. I mean examples and some explanation at serentiy-js.org, not just somehow hidden in some test specs in the repo.

Then what you say is: "It should not require code changes, but anyway, it does." ;)

If I think of our business code bases, this will mean some hundred lines of code changes for typed returns as well:

// from
const ShoppingLists: PageElements = () => ...
// to 
const ShoppingLists: MetaList<PageElement> = () => ...

So, according to semantic versioning this would be a change for Serenity/JS 4 ?

Maybe we should start thinking of providing some https://www.npmjs.com/package/jscodeshift support or something similar?

While I appreciate change and progress, I also know code bases that have to be maintained and it's hard to stroll through release notes hunting down breaking changes, when you have not the possibility to stay up to date with each and every version update,

(I still struggle with the switch to WIDO8, btw.)

@jan-molak
Copy link
Member Author

jan-molak commented Aug 22, 2023

I'm currently in holidays so let me give you just some initial feedback without diving to deep into the code.

Sure thing, I hope you have a nice time off!

First, I always liked the idea of this "smart chaining" in Serenity/JS.

But I always felt a bit uncomfortable with the introduction of this Metathing, as well. I know what a question is, I know what a list is - but a MetaQuestionand a MetaList - needs some more explanation. For me it sounds like something great, such as MegaList, SuperList, HyperQuestion ;). But at the end this are just object names, right?

HyperQuestion has a nice ring to it 🧐 More seriously though, now that we talk about it, I wonder if "meta-" is still necessary. So originally, we had Targets to represent web elements, targets were not questions, just something completely different because of historical reasons related to the Java implementation.

Over time, targets became questions themselves, and the model became better aligned.

Meta-questions were meant to be "questions that can be parameterised with other questions", but now that I think about it more, I wonder if this distinction still holds.

Warning: Brain dump commencing.

Does it make sense to have a question without its context? A PageElement is always resolved in the context of some root page element. Text, Value, Attribute and so on are always "parameterised" with a page element that's also a question.

So would it be fair to say that every question can be parameterised with a question, and therefore every question is a meta-question and so the distinction is no longer necessary?

I wonder how this would work with questions like notes and LastResponse.status() and so on. This will require a deeper dive.

Explanation of https://serenity-js.org/api/core/interface/MetaQuestion/ always sounded a bit stiff to me.

Yeah, it might be trying to tell us something :)

Okay, back to this PR.

What comes into my mind: We should have some clear examples what the goal and the possibilities of this chaining is. I mean examples and some explanation at serentiy-js.org, not just somehow hidden in some test specs in the repo.

Yeah, agreed 100%. In fact, I'm re-working the handbook as we speak.

Do you feel like there is a natural place for those examples in the docs already? Should we extend the article on PEQL, or do we need something different?

Then what you say is: "It should not require code changes, but anyway, it does." ;)

Hmm... yeah. You might be right here.

If I think of our business code bases, this will mean some hundred lines of code changes for typed returns as well:

// from
const ShoppingLists: PageElements = () => ...
// to 
const ShoppingLists: MetaList<PageElement> = () => ...

Yeah, I focused on folks using PageElements to specify the type of function argument, and there aren't that many of those examples in the codebases I see. However, you're right, I forgot about function return types. Drats.

So, according to semantic versioning this would be a change for Serenity/JS 4 ?

I wonder if I can make this change without touching the public API. Let me have a think.

I can do it in a backwards-compatible way. Good catch and thank you!

Maybe we should start thinking of providing some https://www.npmjs.com/package/jscodeshift support or something similar?

This could be useful, but I have no capacity to research that. If you have any time to do it that would be awesome!

While I appreciate change and progress, I also know code bases that have to be maintained and it's hard to stroll through release notes hunting down breaking changes, when you have not the possibility to stay up to date with each and every version update,

Yup, point taken.

(I still struggle with the switch to WIDO8, btw.)

Oh, is this something that's WDIO8-specific, or do we have any bugs in S/JS?

@jan-molak jan-molak changed the title feat(web): replaced PageElements with MetaList<PageElement> feat(web): PageElements chaining Aug 22, 2023
@jan-molak jan-molak changed the title feat(web): PageElements chaining feat(web): better chaining for PageElements Aug 22, 2023
@jan-molak jan-molak changed the title feat(web): better chaining for PageElements feat(web): chainable PageElements Aug 23, 2023
@jan-molak jan-molak merged commit 4d0c7eb into main Aug 23, 2023
4 checks passed
@jan-molak jan-molak deleted the features/chainable-meta-questions branch August 23, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@serenity-js/core @serenity-js/web Screenplay Pattern APIs for interacting with the Web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants