-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Moving logic from templates to libs #2989
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
base: main
Are you sure you want to change the base?
Conversation
0770d1f to
679a882
Compare
Signed-off-by: Mihovil Ilakovac <[email protected]>
679a882 to
b4e1769
Compare
Deploying wasp-docs-on-main with
|
| Latest commit: |
7c08e2d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://461bd450.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://templates-to-libs.wasp-docs-on-main.pages.dev |
08f9765 to
72e2676
Compare
72e2676 to
49d979c
Compare
| @@ -1,13 +1,13 @@ | |||
| import { parseCookies } from "@wasp.sh/libs-auth"; | |||
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 made this refactor by mistake not realizing that I'm touching the server and not the SDK. It works because the @wasp.sh/libs-auth is installed in the root node_modules... but it breaks our philosophy that we introduced with 0.12.0: user imports from the SDK -> framework imports user code.
If the server imports the auth lib directly, I consider that importing the SDK since the auth lib is just an implementation detail of the SDK.
So, how do we tackle this? The server doesn't have oslo listed as one of its deps (meaning this worked just because SDK had oslo as a dep).
Do we add oslo as the dep of the server to be precise?
Can we avoid the server using oslo directly? That would mean that the server uses the SDK?
Do we consider server using the auth lib directly a violation of our 0.12.0 philosophy?
cc: @sodic
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 a straightforward solution would be to have libs-auth-server and libs-auth-sdk etc
Or the same library with multiple exports should be fine, no?
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've encoded this worry in this issue as we'll need to figure out the naming: #3069
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.
Latest status: I've implemented the proposal from the linked issue in this PR #3069
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 like the idea of not "dragging" the server's import chain through the SDK if it isn't necessary. Since SDK will become heavily templated, we want to decouple it from the rest of our codebase as best we can, which means minimizing SDK exports into the framework code.
the SDK since the auth lib is just an implementation detail of the SDK.
This makes sense too because framework code importing stuff from multiple locations seems messy, and I'm 90% sure it will have to import templated stuff at some point.
I'll have to think about this some more, we can discuss it on a call.
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.
@infomiho I went through all the comments and commented again to those that needed it!
I think we are are tehre 99% on my side. Most of the comments have RAW in them, so please check them out and then resolve them so they don't clutter the convo.
There are only a few that might need more communication, but I thikn we can get those done quickly.
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.
Approving this PR as in:
- Did a mid-depth review, I think we're covered on deep ones
- Downloaded it and checked that it works
- Happy with the solution design
- Green light to merge once you address the outstanding comments
I saw that you agreed with all my comments, so feel free to resolve them once they're fixed. I will unsubscribe myself from this PR now but you can @ tag me if you need my input anywhere.
|
@sodic ready for another look! @cprecioso I have only one of your comments to resolve (using WaspLibs in the @Martinsos responded to all of your comment and resolved the ones marked with RAW if it was all clear |
|
@infomiho fine by me. I take it this comment will also be part of that 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.
ONly one comment left on my side but it is RAW, so approving!
High-level overview
This PR:
waspc/libsdirectorydata/libsdirectory.wasp/outalongside the SDK directory so the SDK can import the libsauthlibrary in thelibsdirectory (named@wasp.sh/libs-auth)tsdownnpm packand copy it into thedatadiroslo./sdkand./sdk/browserto be able to separate server and browser exportsWaspLibwhich is how we refer to the libraries in Haskell.tgzfiles) and where to copy themWaspLib(e.g.file:../../lib-1.0.0.tgz)versionto be fixed to0.0.0since it will be replaced with the lib checksum when copiedauthlibrary in the SDK and the serverjwt,cookiesandpasswordhelpers, these helpers depend on external deps e.g.osloand@node-rs/argon2(native deps)./sdk/browserentry to show we can split the codeNote
Note for reviewers: there are three sections:
authlibraryauthlibrary is used in the SDKI'd suggest reviewing the
authlibrary last since it's just a JS project, nothing new to see there.How to test it locally
./tools/install_packages_to_data_dir.shwaspc/examples/todoAppwith thecabal runor your alias of choice e.g.wrunI've tested it in development and production build modes and it works as expected ✅
Known issues in development
You have to delete thenode_modulesand the@wasp.sh/libs-authentry frompackage-lock.jsonto install the new development lib version that you built locally.EDIT: fixed by using a checksum of the library's
.tgzchecksum as the version which preventsnpmfrom using a stale version. Also, I've excluded the libs from being optimized (and then becoming stale) by Vite in development mode.Left to do
.tgzfiles. I'll do this in a separate PR to keep it easier to review this one. Ignoretgzfiles in e2e tests #3078waspc/README.mdand inrunscriptCloses #2936
Closes #3069