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

fix(platform) instanceof bug between execution contexts of RegExp object #1048

Merged
merged 9 commits into from
Feb 25, 2020

Conversation

aesyondu
Copy link
Contributor

@aesyondu aesyondu changed the title Fix instanceof bug between execution contexts of RegExp object fix(platform) instanceof bug between execution contexts of RegExp object Feb 17, 2020
src/platform.ts Outdated Show resolved Hide resolved
@aesyondu
Copy link
Contributor Author

Also, not sure if this should be modified as well?

if (helper.isString(urlOrPredicate) || urlOrPredicate instanceof RegExp)

if (helper.isString(urlOrPredicate) || urlOrPredicate instanceof RegExp)

Use single quotes.
@aesyondu
Copy link
Contributor Author

Not sure how to resolve typescript lint error ##[error]src/platform.ts(226,18): error TS2339: Property 'test' does not exist on type 'RegExp | ((url: URL) => boolean)'.

@JoelEinbinder
Copy link
Contributor

This is because jest overwrites all of the globals right? Would instanceof /./.constructor work then, guaranteeing we get the right regex for this context, not the improper global one. At the same time, jest-playwright should probably add RegExp to the list of globals not to override in jest.

@JoelEinbinder
Copy link
Contributor

Not sure how to resolve typescript lint error ##[error]src/platform.ts(226,18): error TS2339: Property 'test' does not exist on type 'RegExp | ((url: URL) => boolean)'.

typescript does't narrow the type to a regex now that the check is not simple. Cast the variable to a regex before using it. (match as RegEx).test

@msftclas
Copy link

msftclas commented Feb 18, 2020

CLA assistant check
All CLA requirements met.

@aesyondu
Copy link
Contributor Author

aesyondu commented Feb 18, 2020

I modified the logic and refactored it to helper.ts

Where do I add unit tests? I tried searching unit tests for isNumber and isString but can't find them.

@pavelfeldman
Copy link
Member

Where exactly do we pass regex object between contexts? Is this related to the workaround @aslushnikov added before WebKit started accepting multiple collients over the WebSocket?

@JoelEinbinder
Copy link
Contributor

No it’s because jest overrides all globals by default via the vm package.

@JoelEinbinder
Copy link
Contributor

I modified the logic and refactored it to helper.ts

Where do I add unit tests? I tried searching unit tests for isNumber and isString but can't find them.

We always test from the public api. You can use the vm package to get a regex from another context, and verify that you can pass it in as the url match.

@aesyondu
Copy link
Contributor Author

This is because jest overwrites all of the globals right? Would instanceof /./.constructor work then, guaranteeing we get the right regex for this context, not the improper global one. At the same time, jest-playwright should probably add RegExp to the list of globals not to override in jest.

Btw, if I understood you correctly, I tried:

      const ctx = vm.createContext();
      const regexp = vm.runInContext('new RegExp(/digits\\/\\d\\.png/)', ctx);
      console.log(regexp instanceof RegExp, regexp.constructor instanceof RegExp)

but the output was false, false.

@aslushnikov
Copy link
Contributor

For the record: this already happened to us here - #808 (comment)

but the output was false, false.

Which is exactly what we test against here!

@aesyondu, @JoelEinbinder Is this in a good shape for landing, or am I missing something?

src/helper.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JoelEinbinder JoelEinbinder left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the patch!

@JoelEinbinder JoelEinbinder merged commit fdfec8e into microsoft:master Feb 25, 2020
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.

5 participants