-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
transpile ts to js to handle ts types #4133
base: main
Are you sure you want to change the base?
Conversation
Status
|
The PR title and commit don't seem related. Could you update this please |
Addressed |
dd9e7e0
to
f98ffdb
Compare
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.
We would also need to make sure that in case user has a ts config files with name nightwatch-ts.conf.js
(which is a non-default name) and we pass the name of the config using the --config
flag while running tests, it is able to transpile this config as well.
Right now, it won't work because in case a --config
flag is provided, the flow will go to the else
block (which contains the code this.argv.config = path.resolve(this.argv.config);
) and there we do not have any way to transpile the ts file.
}; | ||
const transpiledCode = ts.transpileModule(tsFileContent, {compilerOptions}); | ||
const jsCode = transpiledCode.outputText; | ||
var Module = module.constructor; |
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.
Instead of this, we can also put a const Module = require('module');
statement at the top of the function definition (where other requires are done).
const transpiledCode = ts.transpileModule(tsFileContent, {compilerOptions}); | ||
const jsCode = transpiledCode.outputText; | ||
var Module = module.constructor; | ||
var m = new Module(); |
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.
We should use const
instead of var
for declaring variables. Also, we should define proper variable names, module
would be a better name instead of m
here.
@@ -354,7 +372,11 @@ class CliRunner { | |||
newConfigCreated = CliRunner.createDefaultConfig(localJsOrTsValue); | |||
} | |||
|
|||
if (hasJsOrTsConfig || newConfigCreated) { | |||
if (Utils.fileExistsSync(CliRunner.CONFIG_FILE_TS)) { |
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.
We should ideally use localJsOrTsValue
here as our source of truth to decide if the ts config file is our primary config for the project. This is because the logic used to find localJsOrTsValue
may change later and we may give preference to some other config file even if a nightwatch.conf.ts
file is present.
This could be solved by changing this statement to:
localJsOrTsValue.includes(path.basename(CliRunner.CONFIG_FILE_TS))
And for the issue you were facing with tests, that could be solved by changing the basename
definition in mockery to:
basename(a) {
if (a === './globals.json') {
return 'globals';
}
return origPath.basename(a);
},
Thanks in advance for your contribution. Please follow the below steps in submitting a pull request, as it will help us with reviewing it quicker.
examples/tests
directory of the project) and running them.ecosia.js
andduckDuckGo.js
are good examples to work with.features/my-new-feature
orissue/123-my-bugfix
);Fixes #3688
Dev Testing Steps:
Test case 1:
Test case 2: