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

Improve imports/deps to use playwright-core transitively and Improve Example naming #158

Closed
wants to merge 2 commits into from

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Dec 2, 2020

This patch aims to help reduce the number of deps a user's suite would need by:

  1. Making playwright-core a transitive dep
  2. Using the Page object from playwright-core instead of playwright-chromium which is more correct
  3. Updating the todos example package.json to no longer reference playwright as a dep, but only synthetics, from which playwright deps
    come transitively
  4. This also improves the example file naming in an effort to help docs changes in the future

This is a prereq for improving the docs for elastic/observability-docs#284

This patch aims to help reduce the number of deps a user's suite would
need by:

1. Making playwright-core a transitive dep
2. Using the `Page` object from `playwright-core` instead of
`playwright-chromium` which is more correct
3. Updating the todos example `package.json` to no longer reference
`playwright` as a dep, but only `synthetics`, from which playwright deps
come transitively
@andrewvc andrewvc added the enhancement New feature or request label Dec 2, 2020
@andrewvc andrewvc changed the title Improve imports/deps to use playwright-core transitively Improve imports/deps to use playwright-core transitively and Improve Example naming Dec 2, 2020
@apmmachine
Copy link

apmmachine commented Dec 3, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Branch indexing

  • Start Time: 2020-12-17T10:04:25.345+0000

  • Duration: 15 min 37 sec

Test stats 🧪

Test Results
Failed 0
Passed 46
Skipped 0
Total 46

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewvc I am not 100% sure if i understand the PR,

We are using the page object from playwright-core for typings

import { Page } from 'playwright-core';

playwright-core does not install any browser stuff, So we can still keep it as its harmless (not attempting to replace any global browser packages) and remove it from our package.json as its unnecessary module.

@andrewvc
Copy link
Contributor Author

andrewvc commented Dec 3, 2020

@vigneshshanmugam The idea here is to let users have as minimal a package.json as possible as a best practice for users in their own suites.

Today, the todos folder has its own package.json that explicitly declares playwright-core dep, with this change that's no longer necessary. The nice thing about that is that it puts us firmly in control of the playwright version.

If we ask users to specify an @elastic/synthetics version and a playwright-core version then there could be situations where our exact version dep goes out of sync with theirs. So, my goal here was to just have @elastic/synthetics as a dep with no need for users to specify a playwright version at all.

@@ -40,6 +40,7 @@
"commander": "^6.0.0",
"http-proxy": "^1.18.1",
"kleur": "^4.1.1",
"playwright-core": "=1.6.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, the examples works as node_modules are traversed from local -> parent -> root directory.

We can get rid of playwright-core deps totally and just not use any deps in the example. We are just importing them for making intensense work.

Lets change this to

import { Page } from 'playwright-core';
import { Page } from "playwright-chromium" if we need auto completion or we can even get rid of those and keep the example minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vigneshshanmugam well, intellisense working is an important feature. We should discourage users in our examples from using playwright-chromium because that's not correct for multi-browser support (and confusing).

Additionally, one of the problems with our examples folder is the traversal to the root directory. Users will use the todos example in their projects, so everything in the todos folder should work even if it's removed from the context of our synthetics parent project. In fact, I've been thinking it'd be much better if we could make todos a sibling folder instead of a subfolder for this very reason.

@vigneshshanmugam
Copy link
Member

@andrewvc playwright-core does not install or download any browser versions so we shouldn't even need that in the first place and the versions will not go out of sync. This change feels unnecessary to me, just changing the import {Page} from "playwright-core" and removing that deps is good enough to improve the examples.

@vigneshshanmugam
Copy link
Member

Closing in favor of #239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants