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(mssql/async-queue): fix unable to start mysql due to circular ref #13823

Merged
merged 2 commits into from Dec 28, 2021
Merged

fix(mssql/async-queue): fix unable to start mysql due to circular ref #13823

merged 2 commits into from Dec 28, 2021

Conversation

zorji
Copy link
Contributor

@zorji zorji commented Dec 24, 2021

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Fix unable to start mysql docker in test due to circular reference

Why this happen? Because on this line, it attempts to export an Error that's outside errors dir and this outside AsyncQueueError has circular reference to errors/index.ts

export { AsyncQueueError } from '../dialects/mssql/async-queue';

It's a bit weird to me that errors/index.ts attempts to expose an Error that's outside this dir, maybe it shouldn't do it in the first place but removing it will be a breaking change so I'll not change it in this PR. Taking it out requires a different discussion.

Steps to reproduce

Node: v14.18.1

run the follow command

npm run start-mysql 

and received the following error

class AsyncQueueError extends import_errors.BaseError {
                                            ^

TypeError: Class extends value undefined is not a constructor or null

@WikiRik
Copy link
Member

WikiRik commented Dec 24, 2021

Thanks for the Pull Request! Are there any tests that we can add to this?

@zorji zorji marked this pull request as draft December 25, 2021 00:34
@sdepold
Copy link
Member

sdepold commented Dec 26, 2021

Hm. I just started the MySQL setup and it worked perfectly fine. Will have another look tomorrow

@sdepold
Copy link
Member

sdepold commented Dec 27, 2021

v14.18.1
[main][~/Projects/sequelize]$ yarn build
yarn run v1.22.17
$ node ./build.js
Compiling sequelize...
✨  Done in 1.86s.
[main][~/Projects/sequelize]$ yarn start-mysql
yarn run v1.22.17
$ bash dev/mysql/5.7/start.sh
++ dirname -- dev/mysql/5.7/start.sh
+ cd -P -- dev/mysql/5.7
+ docker-compose -p sequelize-mysql-57 down --remove-orphans
Removing network sequelize-mysql-57-network
WARNING: Network sequelize-mysql-57-network not found.
+ docker-compose -p sequelize-mysql-57 up -d
Creating network "sequelize-mysql-57-network" with the default driver
Creating sequelize-mysql-57 ... done
+ ./../../wait-until-healthy.sh sequelize-mysql-57
sequelize-mysql-57 is healthy!
+ docker exec sequelize-mysql-57 mysql --host 127.0.0.1 --port 3306 -uroot -psequelize_test -e 'GRANT ALL ON *.* TO '\''sequelize_test'\''@'\''%'\'' with grant option; FLUSH PRIVILEGES;'
mysql: [Warning] Using a password on the command line interface can be insecure.
+ node check.js
+ echo 'Local MySQL-5.7 instance is ready for Sequelize tests.'
Local MySQL-5.7 instance is ready for Sequelize tests.
✨  Done in 20.71s.```

@sdepold
Copy link
Member

sdepold commented Dec 27, 2021

That is on master. Not sure if we need to fix anything?!

@joaoe
Copy link
Contributor

joaoe commented Dec 28, 2021

If lib/errors/index.ts must have AsyncQueueError then that class should move to the errors or dialect/abstract folders. Doesn't make sense to expose something that is dialect specific to the rest of the world.

@joaoe
Copy link
Contributor

joaoe commented Dec 28, 2021

Thanks for the Pull Request! Are there any tests that we can add to this?

Well, the test is that sequelize can be imported and runs :) Because a circular ref breaks everything. I'm not sure how this kind of error was merged to the main branch undetected.

@sdepold
Copy link
Member

sdepold commented Dec 28, 2021

As much as I cannot reproduce this issue, I think the change makes sense in general

@sdepold sdepold marked this pull request as ready for review December 28, 2021 20:30
@sdepold sdepold merged commit 2a2cc05 into sequelize:main Dec 28, 2021
sdepold added a commit that referenced this pull request Dec 28, 2021
…#13823)

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
@sdepold
Copy link
Member

sdepold commented Dec 28, 2021

Released in 6.12.4

@zorji
Copy link
Contributor Author

zorji commented Dec 29, 2021

Hi @sdepold @joaoe

Merry Xmas! Thanks for reviewing this PR.

I did notice the build did not fail in CI but I didn't have much time to investigate further.

I tried again today I notice I can reproduce the error with npm install but I couldn't reproduce with yarn.

Then I think it's an issue with lock file because there isn't an package-lock.json so all packages installed by npm are latest.

Then I tried to upgrade typescript and esbuild and found I can reproduce it with latest esbuild@0.14.9 and might be related to this reported issue. evanw/esbuild#1427

To reproduce with npm

rm -rf node_modules
git checkout 0a4fbb0^ # << the commit before mine
npm i
npm run build
npm run start-mysql
node dev/mysql/5.7/check.js

To reproduce with yarn

rm -rf node_modules
git checkout 0a4fbb0^ # << the commit before mine
yarn
yarn add esbuild@latest
yarn build
yarn run start-mysql
node dev/mysql/5.7/check.js

@zorji zorji deleted the fix-start-mssql branch December 29, 2021 01:14
@joaoe
Copy link
Contributor

joaoe commented Dec 29, 2021

Merry Xmas! Thanks for reviewing this PR.

Likewise.

Then I tried to upgrade typescript and esbuild and found I can reproduce it with latest esbuild@0.14.9 and might be related to this reported issue. evanw/esbuild#1427

Right. That is what I did, npm install. The newer esbuild also caused another problem with exports.

Cheers.

@sdepold
Copy link
Member

sdepold commented Dec 29, 2021

Cash you confirm that it works in the new version?

@zorji
Copy link
Contributor Author

zorji commented Dec 29, 2021

Cash you confirm that it works in the new version?

Yes, the change in this PR works with the new version.

@sdepold
Copy link
Member

sdepold commented Dec 29, 2021

Sweet

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2022

🎉 This PR is included in version 7.0.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2022

🎉 This PR is included in version 7.0.0-alpha2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
…sequelize#13823)

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants