-
Notifications
You must be signed in to change notification settings - Fork 43
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
Make Examples runnable in heartbeat / docker #98
Conversation
@@ -80,14 +81,24 @@ async function prepareSuites(inputs: string[]) { | |||
const source = await readStdin(); | |||
loadInlineScript(source); | |||
} else { | |||
// eslint-disable-next-line @typescript-eslint/no-var-requires | |||
require('ts-node').register({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigate if this is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in fact required, otherwise ES6 imports do not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try with adding tsconfig.json
to files? Also now that we have decided to have package.json
for all projects. Its better to remove this dependency from our application and let users configure it if they want to work on TS project. We will end up with these situation for sure in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add this for now. If we can have one less file cluttering up our user's folders I think that's an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a suggestion since we are duplicating the logic across tsconfig.json and ts-node register.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really duplicated however, the ts-config.json
only applies to compiling our project, whereas the register
call applies to projects invoked by npx @elastic/synthetics
. If you remove the register
call the todos
example will fail to compile/run in the docker container because typescript does not have ES6 module support by default, for instance, and the tsconfig.json
file will not be in effect.
@@ -58,7 +58,8 @@ async function prepareSuites(inputs: string[]) { | |||
*/ | |||
const pattern = program.pattern | |||
? new RegExp(program.pattern, 'i') | |||
: /.journey.([mc]js|[jt]s?)$/; | |||
: /.+\.journey\.([mc]js|[jt]s?)$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is more correct in that it actually requires the dots to be dots, and not just any char.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just went fast through the code, Going to take it for a spin to test the DX.
@@ -80,14 +81,24 @@ async function prepareSuites(inputs: string[]) { | |||
const source = await readStdin(); | |||
loadInlineScript(source); | |||
} else { | |||
// eslint-disable-next-line @typescript-eslint/no-var-requires | |||
require('ts-node').register({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try with adding tsconfig.json
to files? Also now that we have decided to have package.json
for all projects. Its better to remove this dependency from our application and let users configure it if they want to work on TS project. We will end up with these situation for sure in the future.
@andrewvc Do you mind if i push some cleanups/changes to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This PR aims to make the examples runnable in docker by fixing a number of things. It also replaces all the examples with a simple TODOS app provided by
vue.js
which better demonstrate how to use playwright in a real-world situation.As a follow-up to this PR we should add E2E tests that validate that the examples actually run. I initially wanted to do that in this PR, but I'm OK testing that elsewhere.