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(api-object): cross-module-version instanceof is broken #1112

Merged
merged 6 commits into from
Mar 13, 2023

Conversation

spion
Copy link
Contributor

@spion spion commented Mar 10, 2023

This pull request fixes the issue with instanceof (#581) by implementing Symbol.hasInstance (thanks to @fikisipi for pointing it out)

Docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance

I welcome any ideas on how to write tests demonstrating the issue is fixed, as to set things up one needs to have multiple versions (or instances) of the same module.

Fixes #581

@spion spion force-pushed the fix/api-object-instanceof branch from b06c472 to 65fc05f Compare March 10, 2023 20:42
This pull fixes the issue with instanceof (cdk8s-team#581) by implementing Symbol.hasInstance (thanks to @fikisipi for pointing it out)

Docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance

Signed-off-by: Gjorgji Kjosev <gorgi.kosev@gmail.com>
@spion spion force-pushed the fix/api-object-instanceof branch from 65fc05f to 364e395 Compare March 10, 2023 20:45
Copy link
Member

@iliapolo iliapolo left a comment

Choose a reason for hiding this comment

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

Thanks @spion!

Yeah I think testing the multi version module scenario is pretty crazy to do in a unit test. I think that we can let it go without it

👍

src/api-object.ts Show resolved Hide resolved
src/api-object.ts Show resolved Hide resolved
@iliapolo iliapolo changed the title fix: cross-module-instance ApiObject instanceof fix(api-object): cross-module-version instanceof is broken Mar 12, 2023
Copy link
Member

@iliapolo iliapolo left a comment

Choose a reason for hiding this comment

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

Sweet. Thanks @spion !

(notice there are still some build failures)

Signed-off-by: Gjorgji Kjosev <gorgi.kosev@gmail.com>
@spion spion force-pushed the fix/api-object-instanceof branch from fbb3355 to 726d98c Compare March 13, 2023 19:21
@iliapolo iliapolo added the backport-to-1.x Backport a PR to the 1.x branch label Mar 13, 2023
@mergify mergify bot merged commit fea71cc into cdk8s-team:2.x Mar 13, 2023
cdk8s-automation pushed a commit that referenced this pull request Mar 13, 2023
This pull request fixes the issue with `instanceof` (#581) by implementing `Symbol.hasInstance` (thanks to @fikisipi for pointing it out)

Docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance

I welcome any ideas on how to write tests demonstrating the issue is fixed, as to set things up one needs to have multiple versions (or instances) of the same module.

Fixes #581

(cherry picked from commit fea71cc)
Signed-off-by: Gorgi Kosev <gorgi.kosev@gmail.com>
@cdk8s-automation
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
1.x

Questions ?

Please refer to the Backport tool documentation

mergify bot pushed a commit that referenced this pull request Mar 13, 2023
…1121)

# Backport

This will backport the following commits from `2.x` to `1.x`:
 - [fix(api-object): cross-module-version `instanceof` is broken (#1112)](#1112)



### Questions ?
Please refer to the [Backport tool documentation](https://github.com/sqren/backport)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-1.x Backport a PR to the 1.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove instanceof use when generating JSON manifest from chart
3 participants