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

Add lolexFromGlobal export #164

Closed
wants to merge 2 commits into from
Closed

Add lolexFromGlobal export #164

wants to merge 2 commits into from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 6, 2018

The first commit wraps the entire source inside of a factory function, and replaces global references with checks on the passed in _global. It then invokes that factory with global || window and exports the result from that call, to make the API stay the same for consumers. Highly recommended to view that diff without whitespace changes (https://github.com/sinonjs/lolex/pull/164/files?w=1).

The second commit just exports that factory function, and adds docs.

The name is horrible, suggestions more than welcome.

Fixes #146

lolex.js Outdated
@@ -2,800 +2,828 @@
(function (global){
Copy link
Member Author

Choose a reason for hiding this comment

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

should the diff from this file be excluded?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It's a build artifact we check in when releasing new versions. We should, of course, try to get this built and added to the sinonjs page release page automatically somehow, but until then this serves as the only non-NPM distribution method.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, cool. removed the changes then 🙂

Readme.md Outdated
@@ -273,6 +273,10 @@ Implements the `Date` object but using the clock to provide the correct time.

Implements the `now` method of the [`Performance`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/now) object but using the clock to provide the correct time. Only available in environments that support the Performance object (browsers mostly).

### `lolex.lolexFromGlobal`

In order to support creating clocks based on fake environments (such as JSDOM), Lolex exports a factory method which takes single argument `global`, which it inspects to figure out what to mock and what features to support. When invoking this function with a global, you will get back an object with `timers`, `createClock` and `install` - same as the regular Lolex exports only based on the apssed in global instead if the global environment.
Copy link

Choose a reason for hiding this comment

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

On the last line, "apssed" should presumably be "passed" and the "if" replaced with "of".

Could you perhaps refer to "separate environments" instead of "fake environments"? The latter description is not particularly accurate for jsdom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! and thanks for the typo callouts 🙂

Readme.md Outdated
@@ -273,6 +273,10 @@ Implements the `Date` object but using the clock to provide the correct time.

Implements the `now` method of the [`Performance`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/now) object but using the clock to provide the correct time. Only available in environments that support the Performance object (browsers mostly).

### `lolex.lolexFromGlobal`
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this has lolex in the name (rather than just fromGlobal or withGlobal?

Copy link
Member Author

Choose a reason for hiding this comment

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

mostly as it kinda made sense in my head to do const lolex = lolexFromGlobal(myGlobal). But I'm totally fine with another name. Any preferences?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like withGlobal or fromGlobal. Looks better IMHO lolex.withGlobal(myJsDom)

If people do static imports and think the contextless names are confusing they can name it whatever they like anyway:
import {withGlobal as lolexFromGlobal} from 'lolex'.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Changes look pretty straightforward and LGTM.

@SimenB
Copy link
Member Author

SimenB commented May 7, 2018

I've tested with this PR in Jest now (jestjs/jest#5171) and it works pretty good!

@benjamingr
Copy link
Member

@fatso83 while I realise I can merge stuff - I think this change is big enough to ask for your opinion :)

@benjamingr benjamingr requested a review from fatso83 May 8, 2018 08:59
Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

This looks fine to me! The factory name strikes me as a bit odd, though.

Readme.md Outdated
@@ -273,6 +273,10 @@ Implements the `Date` object but using the clock to provide the correct time.

Implements the `now` method of the [`Performance`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/now) object but using the clock to provide the correct time. Only available in environments that support the Performance object (browsers mostly).

### `lolex.lolexFromGlobal`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like withGlobal or fromGlobal. Looks better IMHO lolex.withGlobal(myJsDom)

If people do static imports and think the contextless names are confusing they can name it whatever they like anyway:
import {withGlobal as lolexFromGlobal} from 'lolex'.

src/lolex-src.js Outdated
};
}

var defaultImplementation = factory(global || window);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Renamed to withGlobal and squashed

fatso83 added a commit that referenced this pull request May 8, 2018
@fatso83
Copy link
Contributor

fatso83 commented May 8, 2018

Merged as da5505b (had to fix the commit msgs that didn't reflect the naming changes).

@fatso83 fatso83 closed this May 8, 2018
@fatso83
Copy link
Contributor

fatso83 commented May 8, 2018

Released as 2.4.0

@SimenB SimenB deleted the factory branch May 8, 2018 15:40
@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Oh, forgot to change the commit message when squashing. sorry about that!

@SimenB SimenB mentioned this pull request May 15, 2018
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