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

Adds a NodeJS usage test #4

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Adds a NodeJS usage test #4

merged 1 commit into from
Dec 10, 2020

Conversation

Aprillion
Copy link
Contributor

@Aprillion Aprillion commented Dec 7, 2020

failing test for example from readme: mswjs/msw#254 (comment)

@Aprillion
Copy link
Contributor Author

Aprillion commented Dec 7, 2020

ah, I have to rebase the branch on top of current mswj:master, not just Aprillion:master

(done)

@kettanaito kettanaito changed the title Test Adds a NodeJS usage test Dec 7, 2020

test('REAME example', () => {
const scenario = () => {
// TODO: BroadcastChannel is not available in Node 14 nor `jsdom` environment
Copy link
Member

@kettanaito kettanaito Dec 7, 2020

Choose a reason for hiding this comment

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

Would you be interested if I pushed a work-in-progress state I've been working on, where we don't invoke things not available in NodeJS? You could then continue with converting a class into a singleton or discuss any other way to implement storage persistency in NodeJS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably yes, not sure when I would find time to think about it properly...

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely no hurry in this. I've pushed my progress to the master: 994505d

Copy link
Contributor Author

@Aprillion Aprillion Dec 9, 2020

Choose a reason for hiding this comment

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

rebased and using the public property value to make the assertion for now

FTR: the test is passing now, at least on Windows Node v14.13.1 :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO this.value should be enough for storage persistence in Node for now,, enforcing singletons can be discussed in a separate issue whether needed (probably nice to have, but not sure if worth the complexity - if I am inspired by a simple idea to enforce single instance per id, I can write a separate PR later)

* which currently fails => need to update code..
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

A great test. Thank you, @Aprillion!

@kettanaito kettanaito merged commit df97ddf into mswjs:master Dec 10, 2020
@Aprillion Aprillion deleted the test branch December 10, 2020 16:05
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.

2 participants