Route typegen changes - Sveltekit Page/LayoutData mixin + Layout houdini_load Inheritance #673
Conversation
🦋 Changeset detectedLatest commit: d260851 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@sjcobb2022 is attempting to deploy a commit to the HoudiniGraphQL Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @AlecAivazis. Not sure why the test failed for windows in the playwright tests. I will make another commit in to ensure that it wasn't a failed merge or something. I'll format as well in a bit. There are some issues with the current generation since, we it's dependant on the .svelte-kit folder. Since this is this is the case, we either need to call npx svelte-kit sync in our mock dir or seperate the type tests out. |
Ok I found a way to mock the svelte type files. I'll commit this and then you can see. |
|
Ok @AlecAivazis @jycouet, All the tests now pass and the e2e on windows is no longer being funny. I think its all good to go. |
|
Thanks for putting this together @sjcobb2022! I really appreciate the effort. It might take me a few days to dig into this in more detail since it's such a large block of code. Hopefully i can find some time before the weekend |
Most of it is the |
| // add every load to the list | ||
| pageQueries.push(...(houdini_load ?? [])) | ||
| scriptPageExports = exports | ||
| async function walk_types(dirpath: string) { |
There was a problem hiding this comment.
The body of this function looks very simliar to walk_routes, am i missing something?
There was a problem hiding this comment.
I found that walk routes felt overly complex and was trying to deal with too much for this case. Since we are merely copying the svelte types, we are able to glean a lot more information about the structure anyway.
Structurally, I think it would be good to move this into kit.ts
There was a problem hiding this comment.
Like i said, I didn't look extremely closely last night but I will have some time today to do that. In general, the point of walk_routes is to abstract away walk down the directory and the check for all of the different kinds of files. If you're familiar with the visitor pattern, that was the general idea. You'll notice in the block of code you replaced that there is no await fs.stat(childPath)).isDirectory() or even checking for every possible script type.
I'll find some some time today and open an PR on your fork so you can see what I have in mind. Worst case I run into some hangup and answer my own question
|
Awesome. @jycouet pretty easy fix, just needed to check for the proxy file and copy if not. I changes the fs file but I maintained the same format as the other functions (ensuring memory safety during tests or whatever the idea was). I hope this is alright. |
There was a problem hiding this comment.
Thank you @sjcobb2022 for your efficiency, you fixed it really quick 👍
On my side, after testing the feature everything looks good.
|
Really appreciate you verifying this @jycouet ❤️ I'll give it a final once over tomorrow and get this merged finally! |
|
Sorry @AlecAivazis and @jycouet realised a minor bug where the copyFileSync function was not returning. Sorry if this is an issue. |
|
@AlecAivazis Sorry I accidentally closed the pull. Also the windows did it's 6000ms polling thing and needs to be rerun I think. EDIT: reopening the pull made it run again. all good. |
|
Heh i actually restarted the windows one when I saw it didn't pass 😅 I'll definitely have some time to give this a final pass today |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@AlecAivazis Thanks man. |
|
@AlecAivazis I think this is everything? EDIT: The tests pass locally for me I think this was just a freak error. |
* Working on new type generation, commit before merge to 0.17.0
* prepare for merge
* notes for future
* Regex the hell out of this
* {Type}Data is functional
* Working 🎉
* relative imports for load types
* added changeset
* Tests working 🎉 & LoadInput proper generation
* FIX: diffs not merged
* FIX: Format and unnecessary log in tests
* tiny formatting issue that will annpy me if I don't fix it
* Additional fs exports were not necessary, oops
* added comments
* Reorder of type generation
* FIX: Run prettier
* Fix lint
* Using walk_routes
* Formatting, Lint and remove commented old code
* Fixed proxy files, use houdini types in e2e, added copyFileSync to fs
* FIX: Format
* FIX: return from copyFileSync
* FIX: add fs.copyFile, fixed async calls, use internal path
* tidy up changeset
Co-authored-by: Alec Aivazis <[email protected]>
Fixes #608 #653
To help everyone out, please make sure your PR does the following:
pnpm run testsandcd integration && pnpm run testspnpm changesetHi there, here is the initial pull for the new type generation that I've been working on. It currently works for all of the cases that are provided in the e2e tests. I haven't had the time to modify any of the type generation tests yet, so I don't think this will pass CI.
Instead of generating a new file, our types now mirror svelte-kit's types which enables both the inheritance of
houdini_loads in layout groups, and automatically inherits the data of SkelteKits load functions.This should probably be a draft PR as there are probably some code-formatting preferences that need to be looked at, I would appreciate any changes that are made.
P.S. not sure if I updated all the right packages correctly in the changeset. Please feel free to change/fix it :)).