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

chore: update package locks #9545

Closed
wants to merge 1 commit into from
Closed

chore: update package locks #9545

wants to merge 1 commit into from

Conversation

dhmlau
Copy link
Member

@dhmlau dhmlau commented May 18, 2023

Ran npm run update-package-locks to update the package lock files.

cc @samarpanB

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@samarpanB
Copy link
Contributor

Some tests failing. Not sure if those are due to this change or not.

@frbuceta
Copy link
Contributor

I have not seen this PR. I have merge a commit that fixes the update problem but it gives a problem in the Sequelize extension.

@frbuceta
Copy link
Contributor

It's failing here

const data = await this.sequelizeModel.findByPk(

It is a method of Sequelize. See https://sequelize.org/docs/v6/core-concepts/model-querying-finders/#findbypk

@frbuceta
Copy link
Contributor

I have found the problem. Sequelize creates a column of type array in INTEGER. That is what is giving problems.

if (
definition[propName].type === Array ||
['Array', 'array'].includes(definition[propName].type.toString())
) {
// Postgres only
dataType = DataTypes.ARRAY(DataTypes.INTEGER);
}

image

@samarpan-b
Copy link

I have asked @shubhamp-sf to check it.

@shubhamp-sf
Copy link
Contributor

@frbuceta It's actually the underlying package (sqlite3) upgrade in this PR that caused this issue.

I've also reported the same on their repo, please go through the issue summary here: TryGhost/node-sqlite3#1694

I had reported this to @dhmlau in this renovate PR: #9367 (comment)

cc: @samarpan-b

@shubhamp-sf
Copy link
Contributor

Just raised a fix to use the working version of sqlite3 statically (until it gets fixed in the upstream) #9547

Also Diana, @frbuceta already pushed the lock maintenance 3 days back. Doesn't it already includes the changes this PR addresses?

@samarpanB
Copy link
Contributor

@dhmlau after this PR - #9547, tests are now passing. I think this PR is not needed anymore.

@dhmlau
Copy link
Member Author

dhmlau commented May 26, 2023

@samarpanB @shubhamp-sf, should we close this PR then?

@dhmlau dhmlau closed this May 26, 2023
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