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

Update TS Config to support new decorators model definition #267

Closed
wants to merge 5 commits into from

Conversation

johneatmon
Copy link
Contributor

Based on this issue and this issue.

Update tsconfig.json:

  • Add support for experimentalDecorators
  • Bump module and moduleResolution to import @sequelize/core/decorators-legacy declarations properly

Update sscce-sequelize-7.ts:

  • Add new Bar model to demonstrate new model definition example with decorators

@johneatmon
Copy link
Contributor Author

Node 14 tests failed for syntax error (makes sense because ??= is Node 15+, IIRC). Though, curiously, SQLite test for Node 16 failed for the same reason.

Not sure if the other failures are related to my change; if someone wants to take a look and let me know, happy to update my PR. 😊

@WikiRik
Copy link
Member

WikiRik commented Dec 13, 2023

SQLite on Node 16 fails due to a typo, the following line should be using ${{ matrix.node}};

node-version: 14.x

I fixed that in #261 but I forgot to look into the failing tests on there so we didn't merge that yet

@WikiRik
Copy link
Member

WikiRik commented Feb 9, 2024

I've updated #261 and the CI is passing there. Can you base your branch on that PR and see if the CI passes still with your updates?

@johneatmon johneatmon changed the base branch from main to WikiRik/Node-16-18 February 9, 2024 22:58
@ephys ephys deleted the branch sequelize:WikiRik/Node-16-18 February 10, 2024 16:41
@ephys ephys closed this Feb 10, 2024
@WikiRik
Copy link
Member

WikiRik commented Feb 10, 2024

@ephys did you close this on purpose? Or did this get auto-closed because you merged my PR?

@ephys
Copy link
Member

ephys commented Feb 10, 2024

It was closed by the merge of your PR as the target branch was deleted. This PR would need to target main but it's not letting me change it anymore :/

@WikiRik
Copy link
Member

WikiRik commented Feb 10, 2024

Ah yes, that's what I thought. The mobile app you mentioned that you closed as completed.

@johneatmon can you change the target branch to main and reopen this PR?

@johneatmon
Copy link
Contributor Author

johneatmon commented Feb 12, 2024

@WikiRik tbh I'm not sure how to do it with this existing pull, do I have to open a new PR?

@WikiRik
Copy link
Member

WikiRik commented Feb 12, 2024

@johneatmon not sure if you can just update the target branch and reopen. If not, you might need to open a new PR with the same changes.

@johneatmon
Copy link
Contributor Author

@WikiRik re-opened in #272

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.

3 participants