Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat (toolbar): Adding new top level package for web developer toolbar #446
feat (toolbar): Adding new top level package for web developer toolbar #446
Changes from all commits
11befac
4eab955
fc862e8
7b5299b
29b04e2
11e48ec
0e3b19b
98f0807
90e9cf6
f18c963
eabbb3d
efb5817
92ccfa8
f1752ea
8b7aa8e
67bd757
8590b41
bf0ad7d
c54a42a
42667aa
94b6b44
17dee6d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 is a wider question but looking at the monorepo structure, I wonder why generator and toolbar are not in components.
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 agree btw, I feel like toolbar definitely belongs in components
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.
Does it make sense to have the backend and demo-app related scripts in the package.json here. The package.json is copied into the dist. Could the library package.json be clean and the demo-app package.json have the db:migrate client:generate scripts etc.?
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.
Agree. I would even argue that we don't want to package a demo-app with the toolbar. Demo apps should be found in
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.
As per above on scripts, if you cut out the dev dependencies to the demo-app package.json then this would maybe allow you to remove from here? I.e.: are there dev dependencies to build and test the toolbar that live here and dev dependencies to run the demo app that belong in the demo app?
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.
Is this a real dev dependency?
It's probably a remnant of the local dev app you use.
But since we don't checkout the dev app we probably don't need this dev dependency.
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 it need them all - at least it doesn't build without them.
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.
in
src/api/toolbar-typescript.ts
the global registry is being imported:So
electric-sql
needs to be a dependency rather than a dev dependency.Perhaps it should even be a peer dependency.
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.
Noticing now that it is listed as a peer-dependency. Why does it need to be a dev dependency also then? Is it being used in some test somewhere? I don't think there are any tests.