-
Notifications
You must be signed in to change notification settings - Fork 7
BREAKING CHANGE: Rescript 12 support #96
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
BREAKING CHANGE: Rescript 12 support #96
Conversation
|
Thank you for the PR!
Okay! |
oh by the way, do you have an easy way to publish pre-releases on npm? Preferably I'd like to test this against a larger codebase I'm using spice for before merging |
- [x] Js.Json.t type has been replaced with JSON.t - [x] Polyvariant tags no longer require the intermediate Js.Json.tagged type, removing some extra function calls - [x] Usage of Js and Belt in the PPX generated code has been migrated to Stdlib - [x] Updated examples and tests to reflect the above changes
d890744 to
37c1608
Compare
Sounds good to me. Let me figure it out. |
|
@illusionalsagacity The |
c77eafd to
c55e055
Compare
c55e055 to
8d77389
Compare
Thanks! unfortunately I ran into issue #85 when trying to test it |
Depends on green-labs#96 - Introduced a comprehensive benchmark file (Benchmark.res) containing various types including records, variants, polyvariants, and tuples to evaluate ppx performance. - Added types with optional fields, nested records, and large variants to stress test the system. - Created utility functions in NoSpiceAnnotations.mjs for user management and status checking without spice annotations.
Depends on green-labs#96 - Introduced a comprehensive benchmark file (Benchmark.res) containing various types including records, variants, polyvariants, and tuples to evaluate ppx performance. - Added types with optional fields, nested records, and large variants to stress test the system. - Created utility functions in NoSpiceAnnotations.mjs for user management and status checking without spice annotations.
|
I'm also getting a very strange stalling/hanging with rewatch after it gets pretty far into the compilation, and with I'm not sure if the compiler performance is related to this, or just the "antivirus" on my work laptop going crazy. |
Hmm.. Looks quite odd to me. Let me investigate little further about the error message |
Depends on green-labs#96 - Introduced a comprehensive benchmark file (Benchmark.res) containing various types including records, variants, polyvariants, and tuples to evaluate ppx performance. - Added types with optional fields, nested records, and large variants to stress test the system. - Created utility functions in NoSpiceAnnotations.mjs for user management and status checking without spice annotations.
#98 is a known issue. Can we separate the #98 work from merging this PR first? |
| Zora.test("variants with @spice.as", t => { | ||
| let variantEncoded = Variants.t_encode("One"); | ||
| testEqual(t, "encode 하나", variantEncoded, 1); | ||
| testEqual(t, `encode 하나`, variantEncoded, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It's not a big deal, but wondering why " is changed to `?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like on v12 the compiler now emits backtick strings if that how it was written in the .res file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get it now. Thank you for explaining it.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "ppx-spice-examples", | |||
| "name": "spice-ppx-examples", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: need to change the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was giving me a warning from the new rewatch build system, something about breaking package resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it now. Nevermind, I'll fix these names later.
Ah, by 'incorporate' I meant on the build I was using to test; but yeah I can untangle the changes in #98 from the changes here |
Thanks! |
|
I think that |
illusionalsagacity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
@rescript/runtimepackage is needed to run the test in/test. Don't we need to add it as a dev dependency?test git:(illusionalsagacity-rescript-12-support) ✗ pnpm test > spice-ppx-test@1.0.0 test /Users/mununki/GitHub/gl/ppx_spice/test > pta './test/__tests__/Index.mjs' | tap-difflet node:internal/modules/package_json_reader:316 throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), null); ^ Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@rescript/runtime' imported from /Users/mununki/GitHub/gl/ppx_spice/test/src/Records.mjs at Object.getPackageJSONURL (node:internal/modules/package_json_reader:316:9) at packageResolve (node:internal/modules/esm/resolve:768:81) at moduleResolve (node:internal/modules/esm/resolve:858:18) at defaultResolve (node:internal/modules/esm/resolve:990:11) at #cachedDefaultResolve (node:internal/modules/esm/loader:737:20) at ModuleLoader.resolve (node:internal/modules/esm/loader:714:38) at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:293:38) at #link (node:internal/modules/esm/module_job:208:49) at process.processTicksAndRejections (node:internal/process/task_queues:103:5) { code: 'ERR_MODULE_NOT_FOUND' } Node.js v24.11.1
Hmm, that is odd. It's listed in the pnpm lockfile and yarn sees it:
$ pnpm why @rescript/runtime
Legend: production dependency, optional only, dev only
spice-ppx-test@1.0.0 /Users/rob/src/github.com/illusionalsagacity/ppx_spice/test
devDependencies:
rescript 12.0.0
└── @rescript/runtime 12.0.0
$ yarn why @rescript/runtime
yarn why v1.22.22
[1/4] 🤔 Why do we have the module "@rescript/runtime"...?
[2/4] 🚚 Initialising dependency graph...
[3/4] 🔍 Finding dependency...
[4/4] 🚡 Calculating file sizes...
=> Found "@rescript/runtime@12.0.0"
info Reasons this module exists
- "rescript" depends on it
- Hoisted from "rescript#@rescript#runtime"
info Disk size without dependencies: "19.2MB"
info Disk size with unique dependencies: "19.2MB"
info Disk size with transitive dependencies: "19.2MB"
info Number of shared dependencies: 0
What is your $PWD running this commands? |
It works fine after cleaning the node_modules dir. |
mununki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work!
|
Any thing left behind this PR? |
Depends on green-labs#96 - Introduced a comprehensive benchmark file (Benchmark.res) containing various types including records, variants, polyvariants, and tuples to evaluate ppx performance. - Added types with optional fields, nested records, and large variants to stress test the system. - Created utility functions in NoSpiceAnnotations.mjs for user management and status checking without spice annotations.
I don't think so, did you want to merge this one first and then #98? |
I'd like to merge this PR first, then performance improvement such as #98 if you don't mind. Therefore, can you separate this commit illusionalsagacity@5177e87 to another PR? I'd like to minimize this PR and focus to the support the compiler v12. |
I'm not seeing that commit in this PR, it's in the perf-improvement branch |
|
Oh, I'm confused about the GitHub ui. |
|
@illusionalsagacity I'm also getting |
I did not, the new build system ended up working for me with 12.0.1 and the changes in #98 |
|
@illusionalsagacity Thanks for the reply. Unfortunately we can't use the new build system. I think that Spice cannot be used at all in ReScript v12 with the legacy build system. Am I correct, or do you know of some version that does work in these circumstances? |
Can you provide a reproducible project for debugging? |
|
@mununki Of course, here you go: https://github.com/benadamstyles/rescript-v12-legacy-build-spice-repro |
|
I've added an issue #109 |
| "../ppx" | ||
| ], | ||
| "dev-dependencies": ["@dusty-phillips/rescript-zora"], | ||
| "ppx-flags": ["@greenlabs/ppx-spice/ppx"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to have made a mistake in my review. It seems like ppx-flags should be handled by the locally built ppx executable binary. I'll correct this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to have made a mistake in my review. It seems like ppx-flags should be handled by the locally built ppx executable binary. I'll correct this in a separate PR.
My bad. I found out that the ppx is added as a workspace dependency.
Js.Json.ttoJSON.tresultin addition to the existingBelt.Result.ttypeBelt.Array.getExnin the PPX generated code, is this ok to change toArray.getUnsafe?examplesandtestare automatically linked to the parent folder