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

Bugfix/mikro orm knex upgrade #351

Closed

Conversation

knixeur
Copy link

@knixeur knixeur commented Aug 16, 2023

Had to change where mikro-orm set names query is catch as it seems the new version does not execute it in a single query.

All unit tests passing both in master with knex 0 and mikro-orm 4 and with the new versions.

Relates to #270 #255 #313 #214 #361

Copy link
Contributor

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Great work, this ought to be merged and published ASAP.

My one piece of feedback is that running npm audit --omit=dev on this branch still fails. If I checkout the branch and run npm audit fix, the vulnerability goes away and all tests still pass without any further changes to package.json. I'd recommend this gets done in this PR as well, so that pg-mem can finally be free of vulnerabilities in its dependencies.

@knixeur
Copy link
Author

knixeur commented Aug 24, 2023

Thanks for the heads up @sangaman, I've updated the PR accordingly.

Copy link
Contributor

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@murbanowicz
Copy link

@sangaman could you please approve workflows and get it released please as it's been reviewed and approved already?

@sangaman
Copy link
Contributor

@sangaman could you please approve workflows and get it released please as it's been reviewed and approved already?

I wish I could, but I'm not a maintainer 😄. Maybe this repo isn't being maintained currently and ought to be transferred or forked, but I don't think I have the time/expertise to do that myself.

@murbanowicz
Copy link

@oguimbal ?

@knixeur
Copy link
Author

knixeur commented Sep 20, 2023

Related oguimbal/pgsql-ast-parser#140 to add SET NAMES 'utf8' support and avoid the ugly hack

@knixeur
Copy link
Author

knixeur commented Jan 8, 2024

@oguimbal hey! glad to see you back! are you solving the conflicts or do you want me to do it? AST parser version should be bumped too to remove the ugly set names hack.

@oguimbal
Copy link
Owner

oguimbal commented Jan 9, 2024

If you could resolve it, that would be great :)

Note that mikro-orm depends on knex on runtime
It used to work by just intercepting the query but it now builds a bigger
query which includes this part and breaks the parser.
@knixeur knixeur force-pushed the bugfix/mikro-orm-knex-upgrade branch from b728ace to 556be43 Compare January 9, 2024 15:06
@knixeur
Copy link
Author

knixeur commented Jan 9, 2024

Done :)

Btw, I tried to remove the set names hack but it threw another error so I'll leave it until it can be fixed in the parser

@oguimbal
Copy link
Owner

thanks.

see my comment on #377

@oguimbal
Copy link
Owner

Not sure why the tests did not pass for node 14 though ?

@knixeur
Copy link
Author

knixeur commented Jan 10, 2024

That's weird, I've just tested locally and seems to work fine.

❯ node --version
v14.21.3
❯ npm test

> pg-mem@2.7.4 test /home/gb/projects/gpb/pg-mem
> mochapack src/**/*.spec.ts --webpack-config ./tools/webpack.config.js

 WEBPACK  Compiling...

  [=========================] 100% (completed)

 WEBPACK  Compiled successfully in 7226ms

 MOCHA  Testing...
...
...
  792 passing (5s)
  25 pending

 MOCHA  Tests completed successfully

@knixeur
Copy link
Author

knixeur commented Jan 10, 2024

And... reproduced...
I did the same test but deleted node_modules and package-lock.json and did npm install same node version v14.21.3

 MOCHA  Tests completed with 464 failure(s)

Considering neither Node 14/16 are supported maybe it's time to bump it? https://nodejs.org/en/about/previous-releases

@knixeur
Copy link
Author

knixeur commented Jan 10, 2024

Resolved in #377 integrating ast parser change

@knixeur knixeur closed this Jan 10, 2024
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.

4 participants