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

Update tests to suit DefinitelyTyped standard #7

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Update tests to suit DefinitelyTyped standard #7

wants to merge 6 commits into from

Conversation

wallzero
Copy link

I am going to make a PR to DefinitelyTyped to include your Nightwatch type definition. I've made the following modifications to meet their standard. Feel free to merge.

@rkavalap
Copy link
Contributor

@schlesiger why do we need these changes in DT as well ? I haven't kept myself updated. One of the repo's should be obsolete going forward. Which tools pulls the changes from DT ?

@wallzero
Copy link
Author

I think DefinitelyTyped works differently than Typings. I never worked with Typings and am relatively new to Typescript, but I think Typings referred to other git repositories (like this one) while DefinitelyTyped expects everything in one mono repo. Therefore this repository should be obsolete; but I made this PR anyhow to keep you in the loop as the original author. Now Typescript automatically loads types from node_modules/@types, and you add types in the package.json via @types/nightwatch.

Your definition file is now tracked in master of DefinitelyTyped: Link including the above modifications. I've also made further additions listed in this new PR. I'll add those changes to my fork.

@wallzero
Copy link
Author

What also may have changed since Typings is the typings option in package.json, which Typescript will also automatically locate. This makes it much easier for package maintainers to maintain their own definition file without DefinitelyTyped. If the nightwatch team wanted, they could add this definition to their repository instead - but I assumed if they haven't after a year since you started writing it, they probably aren't interested or don't have the resources to support typescript.

@unional
Copy link
Collaborator

unional commented Aug 25, 2017

All repos in @types are waiting for redirects to happen. It can be made installable using npm install types/*. The format thou, should be in module format instead of script format.

There are many advantages in doing this over DT, but again, we are all waiting for redirects to happen.

@wallzero
Copy link
Author

Thanks @unional I did not know that. In the meantime, should this repository and DefinitelyTyped continue to be synchronized?

Also my changes in this PR were just to fix DefinitelyTyped lint issues, change the folder structure, and some minor definition inconsistencies. I had considered wrapping everything in a namespace but I think that could be a separate PR. I also don't have enough experience with Typescript to know what is the common or preferred pattern. It seems the difference in the tests would be minor:

/* test */
export = tests: NightWatchTests
/* test */
export = tests: NightWatch.Tests

I think it'd be best if there was some way to setup the definition so all we'd have to do instead is:

/* test */
import * as nightwatch from 'nightwatch'

export = tests

But I don't think that is possible given how nightwatch expects a returned object of functions which are passed browser.

@wallzero
Copy link
Author

wallzero commented Sep 1, 2017

Hey @rkavalap, can you take a look at this PR? Since you are the original author and listed as a contributor, it appears it requires your approval. I've made several changes; most recently to fix issues when extending nightwatch assertions and commands.

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

Successfully merging this pull request may close these issues.

4 participants