feat: add prism parser #114

sasamuku · 2024-11-29T05:05:18Z

Summary

Add prism parser for schema.rb.
This is alternative to pegjs.

vs pegjs

Type safe
Easy to understand (because it is a common pattern to parse AST)
Highly scalable (same as pegjs maybe)

Related Issue

Changes

Testing

Other Information

sasamuku · 2024-11-29T05:15:38Z

frontend/packages/db-structure/src/parser/schemarb-prism/parser.ts

+  private extractTableName(argNodes: Node[]): string {
+    const nameNode = argNodes.find((node) => node instanceof StringNode)
+    // @ts-ignore: unescaped is defined as string but it is actually object
+    return (nameNode as StringNode).unescaped.value


unescaped is defined as string but it is actually object.
We might need to fix ruby/prism package. btw, let me use ts-ignore in this pr.

StringNode { nodeID: 3, location: { startOffset: 22, length: 7 }, openingLoc: { startOffset: 22, length: 1 }, contentLoc: { startOffset: 23, length: 5 }, closingLoc: { startOffset: 28, length: 1 }, unescaped: { encoding: 'utf-8', validEncoding: true, value: 'users' } }

MH4GF

I commented lightly!

MH4GF · 2024-11-29T05:26:19Z

frontend/packages/db-structure/src/parser/index.ts

 import { processor as schemarbProcessor } from './schemarb/index.js'
 import { processor as postgresqlProcessor } from './sql/index.js'

-type SupportedFormat = 'schemarb' | 'postgres'
+type SupportedFormat = 'schemarb' | 'postgres' | 'schemarb-prism'


Since the default should be prism, how about the following?

Suggested change

type SupportedFormat = 'schemarb' | 'postgres' | 'schemarb-prism'

type SupportedFormat = 'schemarb' | 'postgres' | 'schemarb-deprecated-pegjs'

I'd be fine with a renaming with a different PR 👍🏻

I'll replace them in another pr 👍

frontend/packages/db-structure/src/parser/types.ts

MH4GF · 2024-11-29T05:32:39Z

The conversion part of AST will be checked in an hour 🙏🏻

sasamuku · 2024-11-29T11:31:30Z

@MH4GF
several commits were pushed, pls review them 😄
ci is failed for some reason, I'll check it out later.

MH4GF

Looks great!
Nice work!!!

MH4GF · 2024-11-29T11:33:26Z

frontend/packages/db-structure/src/parser/schemarb-prism/index.test.ts

+      })
+
+      expect(result).toEqual(expected)
+    })


nice test 😄

MH4GF · 2024-11-29T11:34:11Z

frontend/packages/db-structure/src/parser/schemarb-prism/parser.ts

+  // Since 5.1 PostgreSQL adapter uses bigserial type for primary key in default
+  // See:https://github.com/rails/rails/blob/v8.0.0/activerecord/lib/active_record/migration/compatibility.rb#L377


This is an awesome comment!

frontend/packages/db-structure/src/parser/schemarb-prism/parser.ts

sasamuku · 2024-12-02T02:35:17Z

@MH4GF @hoshinotsuyoshi @FunamaYukina

If anyone has idea, please let me know 🙏
I am getting the following error in frontend-ci.
It looks like importing path is wrong, but it doesn't look so.

 FAIL  src/cli/runPreprocess.test.ts [ src/cli/runPreprocess.test.ts ]
Error: Failed to load url src/schema/factories (resolved id: src/schema/factories) in /home/runner/work/liam/liam/frontend/packages/db-structure/dist/parser/schemarb-prism/parser.js. Does the file exist?

MH4GF · 2024-12-02T02:42:50Z

@sasamuku

It seems to be a pattern that works well during development but causes problems after build.

https://github.com/liam-hq/liam/pull/114/files#diff-a7e513747c262e49f364a0ee804085556c818f22f6cb5e56dc82337b3ff1c432R16-R17

Perhaps it is necessary to make this an absolute reference with .js.
Also, maybe ../schema/factories.js instead of src/schema/factories.

sasamuku · 2024-12-02T03:02:37Z

@MH4GF
You're absolutely right👍
Fix on 5f6d3ef

But there is still some problem on test. I'll handle this💪

sasamuku · 2024-12-02T03:14:30Z

📝 the cli build process seems to have failed.

│ x Build failed in 972ms
│ error during build:
│ [vite]: Rollup failed to resolve import "@liam/ui" from "/Users/XXXX/liam/frontend/packages/erd-core/src/components/ERDRenderer/ERDContent/TableNode/TableNode.tsx".

Cannot find module '@liam/ui' or its corresponding type declarations.

MH4GF · 2024-12-02T03:20:36Z

@sasamuku Maybe this is why it's in devDependencies? 💭

liam/frontend/packages/erd-core/package.json

Line 15 in 2f94337

"@liam/ui": "workspace:*",

sasamuku · 2024-12-02T03:32:10Z

@MH4GF
Wow, Wonderful. Indeed, you are right.
Thanks a lot!

Then, another error happened...

│ x Build failed in 1.62s
│ error during build:
│ ../../node_modules/.pnpm/@[email protected]/node_modules/@ruby/prism/src/index.js (2:9): "readFile" is not exported by "__vite-browser-external", imported by "../../node_modules/.pnpm/@[email protected]/node_modules/@ruby/prism/sr…
│ file: /Users/ryota.sasazawa/works/route06/liam/frontend/node_modules/.pnpm/@[email protected]/node_modules/@ruby/prism/src/index.js:2:9
│ 1: import { WASI } from "wasi";
│ 2: import { readFile } from "node:fs/promises";
│             ^
│ 3: import { fileURLToPath } from "node:url";
│     at getRollupError (file:///Users/ryota.sasazawa/works/route06/liam/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:396:41)
│     at error (file:///Users/ryota.sasazawa/works/route06/liam/frontend/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:392:42)

This is similar to error on #90 maybe.

the CLI build test is failing with a "getRollupError"

MH4GF · 2024-12-02T03:38:14Z

@sasamuku Oh, Perhaps this fix is needed:

https://github.com/ruby/prism/blob/main/docs/javascript.md#browser

One could view it as a bad idea to include it in the build for browsers since this time it is running in the CLI, but it should be better to be able to run it in both.
So, please try to fix it!

hoshinotsuyoshi · 2024-12-02T04:01:24Z

frontend/packages/erd-core/package.json

+    "@liam/ui": "workspace:*",
    "@xyflow/react": "12.3.5",
    "react": "18"
  },
  "devDependencies": {
    "@biomejs/biome": "1.9.3",
    "@liam/configs": "workspace:*",
    "@liam/db-structure": "workspace:*",
-    "@liam/ui": "workspace:*",


( Oh, sorry, but I wonder why the CI results for #108 show success. 💭 )

FunamaYukina

I apologize for the delay in the review🙇‍♀️. I've checked the parsing process and it looks good to me (LGTM). I think we can skip the smoke test portion for now and address it in a separate issue.💭

It seems like there's a WASI-related error...

Some results of running pnpm dev

[plugin:vite:resolve] [plugin vite:resolve] Module "wasi" has been externalized for browser compatibility

hoshinotsuyoshi

Perhaps this fix is needed:
https://github.com/ruby/prism/blob/main/docs/javascript.md#browser

I see, I understand that this is the issue with frontend-ci. I didn’t notice anything else concerning in the implementation!

hoshinotsuyoshi · 2024-12-02T09:40:40Z

frontend/packages/db-structure/src/index.ts

@@ -1,4 +1,3 @@
-export { parse } from './parser/index.js'


Parser is removed from index.ts so that it is not referenced at build.
b0a12dd
thank you @MH4GF 🙏

sasamuku · 2024-12-03T02:23:33Z

frontend/packages/cli/src/cli/smoke.test.ts

+      // NOTE: suppress the following warning:
+      if (!stderr.includes('ExperimentalWarning: WASI is an experimental feature and might change at any time')) {
+        expect(stderr).toBe('')
+      }


Workaround for WASI warning.
We could disable warning on npx command, but it would be better to suppress in case of only this waring.

github-actions bot assigned sasamuku Nov 29, 2024

sasamuku force-pushed the add_prism_parser branch 3 times, most recently from cb7e6da to e3b4857 Compare November 29, 2024 05:22

sasamuku commented Nov 29, 2024

View reviewed changes

sasamuku marked this pull request as ready for review November 29, 2024 05:22

sasamuku requested a review from a team as a code owner November 29, 2024 05:22

sasamuku requested review from hoshinotsuyoshi, FunamaYukina, junkisai and MH4GF and removed request for a team November 29, 2024 05:22

MH4GF reviewed Nov 29, 2024

View reviewed changes

sasamuku mentioned this pull request Nov 29, 2024

Primary key icon #108

Merged

1 task

sasamuku force-pushed the add_prism_parser branch 2 times, most recently from d755752 to b64136d Compare November 29, 2024 11:22

sasamuku requested a review from MH4GF November 29, 2024 11:29

MH4GF approved these changes Nov 29, 2024

View reviewed changes

sasamuku force-pushed the add_prism_parser branch from cdb458b to 5f6d3ef Compare December 2, 2024 02:55

sasamuku force-pushed the add_prism_parser branch from cb22cf9 to e0a271c Compare December 2, 2024 03:37

hoshinotsuyoshi reviewed Dec 2, 2024

View reviewed changes

FunamaYukina approved these changes Dec 2, 2024

View reviewed changes

hoshinotsuyoshi approved these changes Dec 2, 2024

View reviewed changes

hoshinotsuyoshi reviewed Dec 2, 2024

View reviewed changes

sasamuku force-pushed the add_prism_parser branch 3 times, most recently from aef7680 to 94f3c88 Compare December 3, 2024 02:17

sasamuku and others added 15 commits December 3, 2024 11:18

add prism parser

d97ecbc

reduce type assertion with as

dc36f82

fix export

b3dbb49

use ts-expect-error intead of ts-ignore

e17f508

change methods to functions

417c620

refactor schema parser tests to use consistent column definitions

d763f93

use factories to initialize tables and columns

095eccf

add comment for 'bigserial' type for primary key in default

fdaa165

Update docs/packages-license.md

ac11e61

Fix comment on index

218e48e

Refactor schema parser imports to use relative paths

951396c

Move liam ui from devDependencies to dependencies

9414283

Update docs/packages-license.md

dfcb71d

Refactor parser imports to pass build for browser

71a7439

Update docs/packages-license.md

8002530

sasamuku force-pushed the add_prism_parser branch from 94f3c88 to a95ab54 Compare December 3, 2024 02:18

sasamuku commented Dec 3, 2024

View reviewed changes

Refactor CLI smoke tests to suppress experimental warning

e6996b8

sasamuku force-pushed the add_prism_parser branch from ac65c05 to e6996b8 Compare December 3, 2024 02:24

sasamuku added this pull request to the merge queue Dec 3, 2024

Merged via the queue into main with commit 0502d8f Dec 3, 2024
10 checks passed

sasamuku deleted the add_prism_parser branch December 3, 2024 02:28

sasamuku mentioned this pull request Dec 3, 2024

Remove pegjs parser #134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add prism parser #114

feat: add prism parser #114

sasamuku commented Nov 29, 2024 •

edited

Loading

sasamuku Nov 29, 2024

MH4GF left a comment

MH4GF Nov 29, 2024

MH4GF Nov 29, 2024

sasamuku Nov 29, 2024

MH4GF commented Nov 29, 2024

sasamuku commented Nov 29, 2024

MH4GF left a comment

MH4GF Nov 29, 2024

MH4GF Nov 29, 2024

sasamuku commented Dec 2, 2024

MH4GF commented Dec 2, 2024

sasamuku commented Dec 2, 2024

sasamuku commented Dec 2, 2024 •

edited

Loading

MH4GF commented Dec 2, 2024

sasamuku commented Dec 2, 2024

MH4GF commented Dec 2, 2024

hoshinotsuyoshi Dec 2, 2024

sasamuku Dec 2, 2024

FunamaYukina left a comment

hoshinotsuyoshi left a comment

hoshinotsuyoshi Dec 2, 2024

sasamuku Dec 2, 2024

sasamuku Dec 3, 2024

	type SupportedFormat = 'schemarb' \| 'postgres' \| 'schemarb-prism'
	type SupportedFormat = 'schemarb' \| 'postgres' \| 'schemarb-deprecated-pegjs'

		// Since 5.1 PostgreSQL adapter uses bigserial type for primary key in default
		// See:https://github.com/rails/rails/blob/v8.0.0/activerecord/lib/active_record/migration/compatibility.rb#L377

feat: add prism parser #114

feat: add prism parser #114

Conversation

sasamuku commented Nov 29, 2024 • edited Loading

Summary

Related Issue

Changes

Testing

Other Information

Choose a reason for hiding this comment

MH4GF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MH4GF commented Nov 29, 2024

sasamuku commented Nov 29, 2024

MH4GF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasamuku commented Dec 2, 2024

MH4GF commented Dec 2, 2024

sasamuku commented Dec 2, 2024

sasamuku commented Dec 2, 2024 • edited Loading

MH4GF commented Dec 2, 2024

sasamuku commented Dec 2, 2024

MH4GF commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FunamaYukina left a comment

Choose a reason for hiding this comment

hoshinotsuyoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasamuku commented Nov 29, 2024 •

edited

Loading

sasamuku commented Dec 2, 2024 •

edited

Loading