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

Updated types for Lolex to the current version (3) #30309

Merged
merged 7 commits into from Feb 4, 2019

Conversation

zyishai
Copy link
Contributor

@zyishai zyishai commented Nov 6, 2018

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

@typescript-bot
Copy link
Contributor

typescript-bot commented Nov 6, 2018

@zyishai Thank you for submitting this PR!

🔔 @Nemo157 @JoshuaKGoldberg @rogierschouten - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@benjamingr
Copy link

As lolex core - this looks good.

Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Reviewing from just a TypeScript perspective since benjamingr approved the content changes - mostly great, with a few touchup requests.

@@ -215,6 +284,22 @@ export interface LolexInstallOpts {
*
* @param now Current time for the clock, as with lolex.createClock().
* @param toFake Names of methods that should be faked.
* @type TClock Type of clock to create.
* @type InstalledClock Type of clock to create.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: if the generic is no longer here, you can just remove the type line.

*/
export declare function install<TClock extends Clock>(opts?: LolexInstallOpts): TClock;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of removing the <TClock extends Clock>, could you change it to <TClock extends InstalledClock = InstalledClock>? It used to be useful to specify which type of clock it was (browser vs node).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do. But could you explain to me what the = does? I didn't saw syntax like that in the documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are Generic parameter defaults. If the user doesn't provide an explicitly value for the generic, it'll default to whatever that value is. The docs are a little out of date on these newer things 🙁 but it's super nifty!

Come to think of it, maybe the TClock generic type should extend Clock, and the return type be TClock & InstalledClock? Something like (but with better names):

type InstalledClockBase = {
    uninstall: () => void;
    methods: FakeMethod[];
};

function install<TClock extends Clock>(opts?: LolexInstallOpts): TClock & InstalledClockBase;

type InstalledClock = InstalledClockBase & Clock;
const browserClock = install<BrowserClock>(); // BrowserClock & InstalledClock;
const clock = install(); // InstalledClock


browserClock.uninstall();
nodeClock.uninstall();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the browser and node clocks no longer able to to uninstall();?

Choose a reason for hiding this comment

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

browser clocks are able to uninstall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg in Lolex source, the method uninstall() is only added to clocks that created with install(), but clocks that were created with createClock() have no uninstall(). That's why I removed the uninstallation of browserClock and nodeClock and added installedClock (line 31) and called uninstall() on it (line 124).

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Nov 6, 2018
@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Nov 6, 2018
@typescript-bot
Copy link
Contributor

@zyishai One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot
Copy link
Contributor

🔔 @JoshuaKGoldberg - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@thejohnfreeman
Copy link
Contributor

This repository cannot be used directly as a source for the @types/lolex package. I tried checking out @zyishai's branch to see if I could publish it, but the release process is far too complicated if you don't want to publish every single package under the scope @types. Can we get instructions on how to publish just this package? Or can we get this PR merged? @JoshuaKGoldberg ? @benjamingr ? Pretty please? Let me know if you need some help and how I can provide it.

@thejohnfreeman
Copy link
Contributor

I figured out how to cobble it together by hand and will use that while we wait for this PR to be merged.

// package.json
"@types/lolex": "https://github.com/thejohnfreeman/types-lolex.git#master"

@JoshuaKGoldberg
Copy link
Collaborator

@thejohnfreeman this repository will release the new types automatically once this PR is merged. That'll happen automatically once >=1 person has signed off and nobody is requesting changes.

...but taking another glance, it looks like I dropped the ball and should have re-reviewed and approved! 😲 sorry for not coming back to this!

/cc @zyishai

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Feb 3, 2019
@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Feb 3, 2019
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@sandersn sandersn merged commit 61a90f7 into DefinitelyTyped:master Feb 4, 2019
@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants