feat(adapters): Add Drizzle #587

notrab · 2024-01-18T13:56:36Z

WHY are these changes introduced?

Drizzle is popular TypeScript ORM that would benefit Shopify App developers.

WHAT is this pull request doing?

This PR adds the ability to use Drizzle as your storage adapter.

Some things aren't clear from a Drizzle side (it doesn't look to be possible or similar to Prisma). There doesn't appear to be a way that I can find in the Drizzle docs to get the schema from a db instance so that's why the constructor requires 2 args — db and sessions.

I tested this using a local SQLite file (via libSQL) and Drizzle following the documentation in the README.md.

If there are any changes or things incorrect, please let me know.

Type of change

Minor: New feature (non-breaking change which adds functionality)

Checklist

I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
I have added/updated tests for this change
I have documented new APIs/updated the documentation for modified APIs (for public APIs)

packages/shopify-app-session-storage-drizzle/src/drizzle.ts

matteodepalo · 2024-01-18T14:34:18Z

Hi @notrab, thank you for your contribution! We're reviewing this internally and we'll get back to you as soon as we can.

notrab · 2024-01-18T14:43:36Z

Awesome, thanks @matteodepalo. I'm new to using Drizzle but happy to change anything, just let me know.

blanklob · 2024-01-18T16:04:52Z

This will close #560

realmikesolo · 2024-01-18T16:48:18Z

Hello, @notrab and @matteodepalo . I am an active contributor to Drizzle ORM and have some improvements that I'd like to implement for this adapter. These include enhancing type-safety and optimizing SQL operations for better correctness and performance, which I believe will make them more suitable for Drizzle ORM. I suggest we postpone this pull request until I have made these improvements based on this code. What are your thoughts on this?

notrab · 2024-01-18T16:53:38Z

@realmikesolo that would be awesome! We asked a few questions in Discord yesterday but didn't get far, I'd love your help!

I'll get you added to the tursodatabase fork as a contributor! ❤️

realmikesolo · 2024-01-18T17:20:11Z

@notrab, thank you! I'm starting to work on this. For any questions or discussions, you can contact me on Discord at @strattt.

notrab · 2024-01-18T17:24:21Z

@realmikesolo sounds good! It’s probably easier you fork and PR the branch. I’ll ping you on Discord.

update: readme;

realmikesolo · 2024-01-22T14:24:45Z

I have signed the CLA!

notrab · 2024-01-22T14:26:18Z

Hey @matteodepalo, with these changes by @realmikesolo we should be good to continue in the review process ❤️

paulomarg · 2024-01-22T15:25:43Z

Hey folks, thank you for contributing! I'll spend some time digging into this, but on the surface it makes sense to me. One thing I notice is that we don't seem to have any tests yet.

We provide a battery of tests that are shared across all of our adapters (e.g. for SQLite or for PGSQL). We use podman to spin up a postgres container, and then run the batteryOfTests to ensure that all of the adapters are consistently implementing SessionStorage.

You should be able to run that if you run yarn build at the top level to build the test utils that provide the battery of tests.

Please let me know if you have any issues adding the tests, and in the meantime I'll go through the code, though I don't expect I'll have too much to say there :)

packages/shopify-app-session-storage-drizzle/LICENSE.md

packages/shopify-app-session-storage-drizzle/package.json

packages/shopify-app-session-storage-drizzle/README.md

packages/shopify-app-session-storage-drizzle/src/adapters/drizzle-sqlite.adapter.ts

packages/shopify-app-session-storage-drizzle/src/schemas/pg.schema.ts

packages/shopify-app-session-storage-drizzle/src/adapters/drizzle-pg.adapter.ts

* add: drizzle adapters for pg & sqlite; update: readme; * fixes according to review

realmikesolo · 2024-01-23T16:45:21Z

Hi, @paulomarg. I am trying to run tests, but have encountered an issue that jest cannot process ES module syntax, as it seems to be using default configuration parameters. Could you provide more details on how to set up the test workflow to allow Jest to properly handle ES module code?

notrab · 2024-02-13T10:04:02Z

@paulomarg I just cloned main and that has failing tests for me too 🤔

 FAIL  packages/shopify-app-express/src/middlewares/__tests__/ensure-installed-on-shop.test.ts
  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'enableGlobally')

      at jestAdapter (../../node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:36:25)

This was also updated a few days ago:

"@shopify/shopify-app-session-storage-memory": "^3.0.0",

Not sure if it could be related with that package versioning.

I have merged main into this branch and push the dep version update to "@shopify/shopify-app-session-storage-test-utils": "^2.0.0" that was made to other packages a few days about by the github-bot.

@realmikesolo and I would appreciate any help getting whatever failing tests resolved. I tried to revert the changes to the loom config to see if it was that tripping up tests but I get the same result.

paulomarg · 2024-02-13T13:43:44Z

@paulomarg I just cloned main and that has failing tests for me too 🤔

That's so weird! I just created a PR and it worked just fine, including CI, so this isn't happening every time. I wonder if this could be because of the node version you're running? I've tried it with node 18 and 20 myself.

Also, could this be it? jestjs/jest#13259 (comment) - it sounds like the versions of jest and jest-circus need to be aligned.

notrab · 2024-02-13T20:47:02Z

@paulomarg ive merged the latest changes @realmikesolo and I were discussing regarding Prisma and main test fixes. This also includes a MySQL adapter.

notrab · 2024-02-19T09:19:24Z

Hey @paulomarg, I just wanted to follow up to see if there was anything you could do to help us get this over the line?

@realmikesolo and I both have inconsistent tests passing with main as well as this branch.

paulomarg · 2024-02-20T15:26:03Z

Hey! I played around with this error a bit, and strangely I don't get it on main - but I was able to get it to work (not 100% reliably, unfortunately) by adding resolutions for the jest and jest-circus packages to the root dir's package.json file. I'd be OK with that as a solution but I haven't found the silver bullet yet :(

paulomarg · 2024-02-21T18:59:37Z

Quick update: this seems to work for me, I got the tests to pass reliably by adding this to the root package.json:

@@ -26,6 +26,7 @@
     "eslint": "^8.55.0",
     "eslint-plugin-prettier": "^5.1.3",
     "jest": "^29.1.0",
+    "jest-environment-jsdom": "^29.7.0",
     "jest-fetch-mock": "^3.0.3",
     "jest-runner-eslint": "^2.1.2",
     "prettier": "^3.2.4",
@@ -34,6 +35,11 @@
     "tslib": "^2.6.2",
     "typescript": "5.3.3"
   },
+  "resolutions": {
+    "jest": "^29.1.0",
+    "jest-circus": "^29.1.0",
+    "ts-jest": "^29.1.0"
+  },
   "dependencies": {},
   "workspaces": [
     "packages/*"

notrab · 2024-02-22T08:26:52Z

Quick update: this seems to work for me, I got the tests to pass reliably by adding this to the root package.json:

@@ -26,6 +26,7 @@
     "eslint": "^8.55.0",
     "eslint-plugin-prettier": "^5.1.3",
     "jest": "^29.1.0",
+    "jest-environment-jsdom": "^29.7.0",
     "jest-fetch-mock": "^3.0.3",
     "jest-runner-eslint": "^2.1.2",
     "prettier": "^3.2.4",
@@ -34,6 +35,11 @@
     "tslib": "^2.6.2",
     "typescript": "5.3.3"
   },
+  "resolutions": {
+    "jest": "^29.1.0",
+    "jest-circus": "^29.1.0",
+    "ts-jest": "^29.1.0"
+  },
   "dependencies": {},
   "workspaces": [
     "packages/*"

That's great @paulomarg! Do you want to commit that or should I?

paulomarg · 2024-02-22T13:37:17Z

Would you mind doing it and running locally to see if that fixes it for you? That should tell us whether it'll work in CI :)

notrab · 2024-02-22T14:27:59Z

Even more errors on the branch when those are added 🙈

Here are the errors on main:

We'll commit the resolutions to this branch so you can see if they fail.

notrab · 2024-02-23T10:03:48Z

Hey @paulomarg

I've tried with multiple engineers on our side to run the tests locally (with and without the resolutions) and it still fails on main. It could be related to this issue though.

Also, I noticed that loom config is using node 16 and there's a message from GitHub Actions that this isn't supported. If there's any changes to be made there, I can merge any changes you make on that.

paulomarg · 2024-02-23T16:12:07Z

I ended up running into a similar issue here: #626

I believe I have it fixed in a way that will make CI happy, including passing @web3-storage through babel. Hopefully that will fix your issue too if you rebase once we merge 🙏

Might cause a couple of conflicts in your loom config - you should just have to add the drizzle one.

Also, I noticed that loom config is using node 16 and there's a message from GitHub Actions that this isn't supported

We'll be moving away from node 16 in the future, but for now there's no need to change that, thank you!

notrab · 2024-02-24T09:46:07Z

Hey @paulomarg @realmikesolo

I've merged upstream and all tests are passing.

* add: mysql adapter & update: tests * merge upstream * bump shopify packages for drizzle

paulomarg

I tested this out in an app and it worked like a charm! The tests are passing, but we had a few linting errors, could you please fix those so we can merge?

Also: 🎉

paulomarg · 2024-02-26T14:06:01Z

packages/shopify-app-session-storage-drizzle/src/adapters/drizzle-mysql.adapter.ts

Also, something just occurred to me while checking this out: the code for all the adapters seems to be very similar.

I may have asked this before, but is there no way to have a generic type for the db param that we can share across all of the adapters, so we only need to implement them once? Probably ok if there's no easy way to do that, but it would make this adapter easier to maintain in the future.

Unfortunately, we can't create a generic type for the db parameter with Drizzle, but I think I can create a function that will return the proper adapter based on the type of the db parameter. Something like this:

export function createSessionStorage(params: Params) { if (is(params.db, BaseSQLiteDatabase)) { return new DrizzleSessionStorageSQLite( params.db, params.sessionTable as SQLiteSessionTable, ); } else if (is(params.db, MySqlDatabase)) { return new DrizzleSessionStorageMySQL( params.db, params.sessionTable as MySQLSessionTable, ); } else if (is(params.db, PgDatabase)) { return new DrizzleSessionStoragePostgres( params.db, params.sessionTable as PostgresSessionTable, ); } } export type Params = | { db: BaseSQLiteDatabase<'sync' | 'async', any, any>; sessionTable: SQLiteSessionTable; } | { db: MySqlDatabase<MySQLQueryResultHKT, PreparedQueryHKTBase, any>; sessionTable: MySQLSessionTable; } | { db: PgDatabase<PostgresQueryResultHKT, any>; sessionTable: PostgresSessionTable; };

const storage = createSessionStorage({ db: drizzleDb, sessionTable, }); const shopify = shopifyApp({ sessionStorage: storage, // ... });

Would this be suitable, or should we just leave it as it is now?

Hmm, I think that might end up complicating things a bit more. We can re-evaluate to see if people are running into problems with the way things are now.

notrab · 2024-02-26T14:16:22Z

@paulomarg lint errors resolved. I think there was a reason db wasn't a generic but I'll leave that to @realmikesolo to better answer.

PS. Thanks for all your time reviewing and making this PR a reality! Same for @realmikesolo 🥳

paulomarg

I just wanted to say big thanks to @realmikesolo and @notrab for sticking with this and seeing it through.

Hopefully this package will be helpful to folks who prefer to work with Drizzle when they're writing apps!

realmikesolo · 2024-02-26T17:25:30Z

@paulomarg @notrab Thank you very much for your cooperation! It was a pleasure to work on this package with you! 🥳

notrab · 2024-02-27T12:14:29Z

@paulomarg is there a timeline for when this will be published to NPM? We'd love to blog about this and link to the package.

Thanks 🙏🏻

paulomarg · 2024-02-27T13:39:31Z

It will be included in the next release. We don't have a fixed scheduled but we try to release at least every couple of weeks.

blanklob · 2024-03-08T14:23:01Z

Keep us updated when this merged and ready for use.

paulomarg · 2024-03-14T17:33:40Z

Hey folks, we just published v1 of the Drizzle package 🎉

Please let us know if you have any issues using it!

blanklob · 2024-03-15T00:35:59Z

Now all we need is a new docker file to deploy remix app with Drizzle

notrab added 7 commits January 14, 2024 16:57

initial drizzle adapter

d263d5a

temp workaround

c3531c2

sessionsTable

23624bd

docs(readme): sessions table usage

11ea3e9

revert to any for sessionTable

4a882f9

set Date

2b92dc1

changeset

49f643a

notrab requested a review from a team as a code owner January 18, 2024 13:56

github-actions bot added the cla-needed label Jan 18, 2024

notrab commented Jan 18, 2024

View reviewed changes

packages/shopify-app-session-storage-drizzle/src/drizzle.ts Outdated Show resolved Hide resolved

github-actions bot removed the cla-needed label Jan 18, 2024

Merge branch 'main' into shopify-app-session-storage-drizzle

5b5f231

add: drizzle adapters for pg & sqlite; (#1)

bca3bec

update: readme;

github-actions bot added the cla-needed label Jan 22, 2024

github-actions bot removed the cla-needed label Jan 22, 2024

paulomarg reviewed Jan 22, 2024

View reviewed changes

paulomarg added the Waiting for response label Jan 22, 2024

Shopify app session storage drizzle (#2)

e41d691

* add: drizzle adapters for pg & sqlite; update: readme; * fixes according to review

github-actions bot removed the Waiting for response label Jan 23, 2024

bump dep

70bacaf

add: mysql adapter & update: tests (#6)

9373b95

add resolutions

fd781f3

merge upstream

e8e5ddb

Shopify app session storage drizzle (#7)

8bce0b2

* add: mysql adapter & update: tests * merge upstream * bump shopify packages for drizzle

paulomarg reviewed Feb 26, 2024

View reviewed changes

resolve lint errors

5eca45b

paulomarg approved these changes Feb 26, 2024

View reviewed changes

paulomarg merged commit 4053132 into Shopify:main Feb 26, 2024
6 checks passed

notrab deleted the shopify-app-session-storage-drizzle branch February 27, 2024 12:13

andersonjoseph mentioned this pull request Mar 18, 2024

[Feat Request] new Session adapter @shopify/shopify-app-session-storage-drizzle #560

Closed

feat(adapters): Add Drizzle #587

feat(adapters): Add Drizzle #587

Conversation

notrab commented Jan 18, 2024

WHY are these changes introduced?

WHAT is this pull request doing?

Type of change

Checklist

matteodepalo commented Jan 18, 2024

notrab commented Jan 18, 2024 • edited Loading

blanklob commented Jan 18, 2024

realmikesolo commented Jan 18, 2024

notrab commented Jan 18, 2024

realmikesolo commented Jan 18, 2024 • edited Loading

notrab commented Jan 18, 2024

realmikesolo commented Jan 22, 2024

notrab commented Jan 22, 2024

paulomarg commented Jan 22, 2024

realmikesolo commented Jan 23, 2024

notrab commented Feb 13, 2024 • edited Loading

paulomarg commented Feb 13, 2024 • edited Loading

notrab commented Feb 13, 2024

notrab commented Feb 19, 2024

paulomarg commented Feb 20, 2024

paulomarg commented Feb 21, 2024

notrab commented Feb 22, 2024

paulomarg commented Feb 22, 2024

notrab commented Feb 22, 2024 • edited Loading

notrab commented Feb 23, 2024 • edited Loading

paulomarg commented Feb 23, 2024 • edited Loading

notrab commented Feb 24, 2024

paulomarg left a comment

Choose a reason for hiding this comment

paulomarg Feb 26, 2024

Choose a reason for hiding this comment

realmikesolo Feb 26, 2024

Choose a reason for hiding this comment

paulomarg Feb 26, 2024

Choose a reason for hiding this comment

notrab commented Feb 26, 2024 • edited Loading

paulomarg left a comment

Choose a reason for hiding this comment

realmikesolo commented Feb 26, 2024

notrab commented Feb 27, 2024

paulomarg commented Feb 27, 2024

blanklob commented Mar 8, 2024

paulomarg commented Mar 14, 2024

blanklob commented Mar 15, 2024

notrab commented Jan 18, 2024 •

edited

Loading

realmikesolo commented Jan 18, 2024 •

edited

Loading

notrab commented Feb 13, 2024 •

edited

Loading

paulomarg commented Feb 13, 2024 •

edited

Loading

notrab commented Feb 22, 2024 •

edited

Loading

notrab commented Feb 23, 2024 •

edited

Loading

paulomarg commented Feb 23, 2024 •

edited

Loading

notrab commented Feb 26, 2024 •

edited

Loading