Skip to content
This repository was archived by the owner on Apr 13, 2025. It is now read-only.

SQL service #237

Merged
merged 8 commits into from
Aug 1, 2021
Merged

SQL service #237

merged 8 commits into from
Aug 1, 2021

Conversation

sebinside
Copy link
Member

@sebinside sebinside commented Jun 13, 2021

Close #223 by using knex with mysql as default client. Can be used with any other supported client, though.

Progress:

  • Create service client
  • Test service client
  • Add example bundle
  • Add some knex example code w/ explanations
  • Remove mysql from dependecy list, update comments

@sebinside sebinside added enhancement New feature or request minor Quick to implement service Requires changes to a service labels Jun 13, 2021
@sebinside sebinside changed the title Add knex and mysql to sql service. SQL service Jun 13, 2021
@sebinside sebinside marked this pull request as ready for review June 20, 2021 20:50
@sebinside
Copy link
Member Author

Still contains a reference to mysql in the package.json to make the service easy to get started with. Could be removed together with a change to the comment in the description in nodecg-io-sql/package.json:4.

@sebinside sebinside requested a review from hlxid June 20, 2021 20:53
Copy link
Member

@hlxid hlxid left a comment

Choose a reason for hiding this comment

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

Please remove nodecg-io-sql/package-lock.json. It is created when using e.g. npm install sqlite3 inside the sql service. Add your dependencies manually to the corresponding package.json file and the run npm run bs in the repository root (this also needs to be done to update the root package-lock.jn)

If you will add a explanation to the docs on how to install support for other sql dialects please use npm install sqlite3 --no-save. This will create no lockfile because for when a user manually installs a dependency these are not really required anyway.

I don't really care whether we want to include mysql by default. On the one hand it makes it easier to get started, on the other we could make the installation smaller for people not using mysql. I'm fine with both, do want you think is better. We still can change it after merging this PR.

@sebinside sebinside requested a review from hlxid August 1, 2021 15:41
hlxid added a commit to codeoverflow-org/nodecg-io-docs that referenced this pull request Aug 1, 2021
@hlxid hlxid merged commit 704a495 into master Aug 1, 2021
@hlxid hlxid deleted the 223-sql branch August 1, 2021 17:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request minor Quick to implement service Requires changes to a service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sql service
2 participants