mst-gql icon indicating copy to clipboard operation
mst-gql copied to clipboard

explore combining all .base files into a single file

Open chrisdrackett opened this issue 5 years ago • 9 comments

as a way to possibly avoid some circular import issues I wonder if it would be valuable to put all the generated code into a single file.

chrisdrackett avatar Jul 28 '20 18:07 chrisdrackett

I can’t find the thread but there was a discussion about this earlier and I absolutely think it’s the way to go. I have some of it implemented on a branch and wasn’t too hard to do; but I got stuck on the following:

I wanted to figure out if we could somehow more organically allow extensions of a shared base models. So that you didn’t need all the non base files either and you just have a “mstgql.ts” file. Instead of importing each extended class in the base models I wanted to see if we could invert the logic somehow by using a explicit composition file that still allowed for proper Typescript types to propagate.

To do this rather than inferring the types I had to generate explicit interfaces for everything and this was a bit more work that I never completed. Not was I 100% convinced by my strategy yet.

If someone’s interested in pairing on this with me please give me a ping. I think getting it right will make mstgql feel a lot more intuitive to use without so many files. And no more circular import problems!

RXminuS avatar Jul 30 '20 07:07 RXminuS

This was the original thread: https://github.com/mobxjs/mst-gql/issues/119

And lots of related issues https://github.com/mobxjs/mst-gql/issues/190

RXminuS avatar Jul 30 '20 07:07 RXminuS

@Matth10 sorry, I’ve been of the scene a bit so missed your comments on those threads. Would you be interested or have someone on your team interested in pairing on this?

RXminuS avatar Jul 30 '20 07:07 RXminuS

@RXminuS Maybe we should open up a new issue or project to cover the exploratory work around this idea? I think its good to keep this issue small and focused, but I'll all for simplification and eliminating the circular issues if at all possible.

chrisdrackett avatar Jul 30 '20 17:07 chrisdrackett

I second this!

Not even for the circular dependencies (even though it should be the main reason to do this), but for organization purposes as well. Even with a small schema I ended up with so many files that it made me get lost often.

I would love to even a have a separate types.d.ts and enums.ts files or something. I think it could go hand-in-hand with this exploration.

jvcmanke avatar Aug 20 '20 01:08 jvcmanke

I am open to help as well! I don't have that much time, but I really like the library and want to help where possible.

I took a look at the generation code, but since I'm not very familiar with it, I didn't understand it very much.

jvcmanke avatar Aug 20 '20 01:08 jvcmanke

I think this would be a good idea.

I used madge to look at why I had some circular dependencies (causing undefined imports) and found that there were tons of cycles just from base files importing other base files. For my case, a large majority of the cycles were from mst-gql models (like 70 circular deps). This make the code base feel really brittle to me.

It feels extra difficult to solve these because they are in generated files that I can't change manually to fix. I definitely think we could/should solve these when scaffolding.

special-character avatar Oct 06 '20 21:10 special-character

I'm also facing circular import problems with js files. Requires manually editing the base files to move the Selectors, select*() functions, and primitives to a separate file for many models, all of which gets undone when I re-scaffold

techknowfile avatar Jan 19 '21 20:01 techknowfile

Just wanted to resurface this issue with more data (and a proposed workaround for now):

https://github.com/facebook/react-native/pull/34476#issuecomment-1240667794

^^ TLDR; Even if you ignore the warnings, they will still show in your console now.

Potential fix (maybe helpful for other people like me, trying to figure out how to remove from my console on newer versions of Expo):

https://github.com/expo/expo/issues/17773#issuecomment-1218301931

Aryk avatar Sep 07 '23 13:09 Aryk