Skip to content
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

Generate modular manager code #2970

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Generate modular manager code #2970

merged 2 commits into from
Apr 26, 2024

Conversation

simolus3
Copy link
Owner

When modular code generation is enabled, we now split manager classes into separate files (instead of generating them all at the database).

Closes #2967. cc @dickermoshe let me know if you see anything standing out here. I've replaced the firstWhere lookup when resolving a references with just using the table directly. I had to do that because the writer no longer includes all tables for modular builds, do you think that could break anything?

Copy link

🚀 Deployed on https://deploy-preview-2970--moor.netlify.app

@github-actions github-actions bot temporarily deployed to pull request April 24, 2024 08:37 Inactive
@github-actions github-actions bot temporarily deployed to commit April 24, 2024 08:38 Inactive
@dickermoshe
Copy link
Collaborator

dickermoshe commented Apr 25, 2024

Review

This looks great.
As long as all tables are added to this list, it should be fine.
Just makes sure tables with CustomRowClasses are being handles correctly (I think I excluded these from the tables list, but I see you're running the checks for that in the actual generation)

I think It would be wise to add tests for modular generated code that has custom row classes too, being that it's really the only part of the code that is kinda brittle. (I wont throw static errors)

LGTM Otherwise...

Future Changes

On another note, I've been away for the past couple of days and was nervous that 2.18 went out.
Thank god it didn't.
I will open an issue and a PR to fix what was a pretty silly API design mistake

Never mind, turns out it was pretty necessary to do what I did.
It had to do with being a able to filter:

items.filter((f)=>f.category.name("Foo"))

instead of

items.filter((f)=>f.category((f)=>f.name("Foo")))

Adding this would be a bunch of work and wouldn't have to be a breaking change (category could be a callable class instead of a function) so I'll add it to the list of things that we should add to manager in the long run.

Docs Nits

What do think about removing the extensions from the docs?

https://deploy-preview-2970--moor.netlify.app/docs/getting-started/manager/#extensions

Why?
Well the internals of how the manager works are pretty fresh. As more is added they will change pretty drastically.
If the package recommends that people leverage extensions to add things at such a low level, then these changes could be classified as breaking.

@dickermoshe
Copy link
Collaborator

@simolus3
Hold off on publishing any releases. Give me a day or so to analyze the current API and make sure that it isn't prone to frequent breaking changes.

Thanks

@simolus3
Copy link
Owner Author

As long as all tables are added to this list, it should be fine.

But I'm not doing that :) Essentially I'm partitioning the tables across different manager generators to generate one per file. So if there are two files (a.drift and b.dart) defining tables TableA and TableB, we'd do two manager generation runs with one table each.
It seems to work and generate references across files correctly from all the examples and tests that we currently have.

I think It would be wise to add tests for modular generated code that has custom row classes too, being that it's really the only part of the code that is kinda brittle. (I wont throw static errors)

What's the problem with custom row classes and the manager anyway? I think we could at least generate the filter/ordering classes for them since they're only referencing schema columns and not the generated row class, right? It's just inserts and updates that are tricky? But still I imagine that should work with companions.

If the package recommends that people leverage extensions to add things at such a low level, then these changes could be classified as breaking.

Anything currently exposed through a public import (and not annotated with an @internal annotation) is part of the public API, we can't change it in a backwards-incompatible way without a major version change either way.
So we either commit to stabilizing those methods or remove them from the public interface.

@dickermoshe
Copy link
Collaborator

Then this PR is great


When I ran it with custom row classes, it broke, so I disabled it, I didn't really dig too deeply into it. I'll take a look at it again.


I'll hide some internal stuff in the other PR

@simolus3 simolus3 merged commit 9c28a06 into develop Apr 26, 2024
21 checks passed
@simolus3 simolus3 deleted the modular-manager branch April 26, 2024 16:18
@simolus3
Copy link
Owner Author

Thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modular code generation for manager
2 participants