Initial refactor #944
Initial refactor #944
Conversation
There was a problem hiding this comment.
Great to see this come together! I went over the PR at a high level and left some inline comments. I also have a few extra questions:
- Why not put the (non-test) code in
web-local/srcinstead of directly inweb-local? That would also mirror howweb-cloudis setup (basic Svelte layout) - I see you moved to relative imports within the package instead of absolute paths. I don't have a strong preference, but curious to understand the motivation?
- Regarding (2.), there are a bunch of imports from
$web-local/lib/which are still absolute that remain to be converted
I'm sure there'll be loose ends to tie up different places, but the way I see it, we don't have to make it perfect in this PR. Just establishing the workspaces and making sure everything runs as before is the important step, then we can gradually tidy smaller things up in later PRs.
tsconfig.json
Outdated
| "$web-local/lib": ["web-local/lib"], | ||
| "$web-local/lib/*": ["web-local/lib/*"], | ||
| "$web-local/common": ["web-local/common"], | ||
| "$web-local/common/*": ["web-local/common/*"], | ||
| "$web-local/cli": ["web-local/cli"], | ||
| "$web-local/cli/*": ["web-local/cli/*"], | ||
| "$web-local/server": ["web-local/server"], | ||
| "$web-local/server/*": ["web-local/server/*"] |
There was a problem hiding this comment.
Why do we use $ over @? And what do you think about prefix the paths with @rilldata for clarity? Like import { ... } from "@rilldata/web-common". (Prisma also does that: https://github.com/prisma/prisma/blob/main/tsconfig.build.bundle.json)
And is it necessary to duplicate each path with a /* variant?
There was a problem hiding this comment.
The standard to use $ instead of @ comes from sveltekit. Adding @rilldata will possibly conflicts with actual packages. I am not sure about the duplication, it was added by sveltekit init I believe (was present before i joined the old data-modeler).
There was a problem hiding this comment.
Just a note that this can be explicitly changed.
There was a problem hiding this comment.
Looked a little more into this. @rilldata should be a scope that only we can publish to, so conflicts shouldn't be a problem.
The idea of using NPM workspaces is to treat each workspace as a separate package, but with the convenience of paths resolving locally instead of through NPM (many also appear to publish packages to NPM for external consumption). Check out these examples:
- grafana/packages (or for a single package: grafana/packages/grafana-ui)
- prisma/packages
|
Yes makes sense to move stuff to src directory. Regarding the imports, i am not sure why some are relative and some aren't. These were the result of just moving the files into the folder on webstorm. |
|
Nice to see this! Agree with all Benjamin's comments. Regarding imports, there's a VSCode configuration for relative/absolute imports. I'm currently on the default setting "shortest", but maybe the "project-relative" setting is the right way to go. My only preference is for consistency. Is there something similar in WebStorm?
|
|
Updating the aliases before moving code to src fixed the paths to use the aliases. I guess that was the issue initially, i updated the aliases after moving. Regarding the setting for module import. I feel shortest is better. It would be pretty ugly when we import a file in the same directory with the project-relative. |
* Initial refactor * Fixing build and test * Fixing local dev command * Fixing vercel binary * Moving files within src directory * Moving to using @rilldata for module alias * Fixing missing test data files * Fixing npm publish
Things in this PR,