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.

Sign up for GitHub

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

Jump to bottom

Remove instanceof symbol #352

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Remove instanceof symbol #352

merged 7 commits into from
Nov 22, 2021

Conversation

dselman
Copy link
Contributor

@dselman dselman commented Nov 22, 2021 •
edited
Loading

Closes #47

  • Removes custom hasInstance code, which is incompatible with Babel
  • Refactor code and tests to use new methods to determine the runtime types
  • Removes support for Node 12, adds support for Node 16

Flags

  • API breaking change

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

jeromesimeon
jeromesimeon suggested changes Nov 22, 2021
View reviewed changes
Copy link
Member

@jeromesimeon jeromesimeon 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! Just one change request on the metamodel.js file.

packages/concerto-core/lib/introspect/metamodel.js Outdated
@@ -19,1069 +19,1069 @@ const ModelManager = require('../modelmanager');
const Factory = require('../factory');
const Serializer = require('../serializer');

const metaModelCto = `/*
Copy link
Member

Choose a reason for hiding this comment

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

Why changes to this file? That seems unrelated to instanceof ? I would remove those changes to avoid conflicts with the other PRs on the new parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove

@jeromesimeon jeromesimeon self-requested a review November 22, 2021 16:17
jeromesimeon
jeromesimeon approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeromesimeon jeromesimeon merged commit cd7d3b7 into master Nov 22, 2021
@jeromesimeon jeromesimeon deleted the ds-remove-instanceof-symbol branch November 22, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromesimeon jeromesimeon jeromesimeon approved these changes

@mttrbrts mttrbrts Awaiting requested review from mttrbrts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid instanceof which leads to runtime errors with deep npm dependencies
2 participants
@dselman @jeromesimeon