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

Control Import Order #112

Closed
wants to merge 1 commit into from
Closed

Control Import Order #112

wants to merge 1 commit into from

Conversation

RXminuS
Copy link
Collaborator

@RXminuS RXminuS commented Sep 30, 2019

The nature of GraphQL is that it is very easy to end up with circular dependencies. Simply something as

type Node {
    edges: [Edge!]!
}

type Edge {
    from: Node!
    to: Node!
}

can quickly become problematic. mstGQL currently doesn't allow any elegant way of controlling the import order resulting in some funky behaviour. In my opinion I don't think any of the .base files should be separate and would prefer one giant base.generated.ts file as it would avoid a lot of the problems but also make it easier with Typescript types on the resolvers etc. But also realise that it probably would be an unpopular move.

So instead I followed Westrate's excellent blogpost about use of internal.ts barrel files.

This change simply makes sure that all the modules now use import {...} from "./internal" rather than individual files. You can then use something like barrelsby to generate an internal.ts file that simply exports every module. You can then move around items in that file to control the loading order (Enums at the top etc).

This code isn't well tested yet so probably needs a bit of scrutiny before merging, but it works for our repo.

@RXminuS
Copy link
Collaborator Author

RXminuS commented Sep 30, 2019

Oh, realise that I might have copied in the edits as I made them in my code editor which was still on v0.4.x but pasted them into a v0.5.x fork.

Might be easiest to just cherry-pick them over instead, basically all I did was change the generation of multiple import statements to a single import statement for "./internal" that imports all the modules from there.

Also we might want to add the generation of the "./internal" file the first time so that it works out of the box? Or maybe we want to hide this behaviour behind a flag instead?

@zenflow
Copy link
Collaborator

zenflow commented Sep 30, 2019

@RXminuS

I'm sorry, I don't really understand the problem this is trying to solve, so I can't give much of a review, but anyways.

Some changes merged to master after the 0.5 release are reverted by this.. see changes to generate.js in 26b887b

@RXminuS
Copy link
Collaborator Author

RXminuS commented Sep 30, 2019

Yeah I will redo it on the latest master branch. I will also see if I can add an example to show the problem, but it’s basically that SomeModelBase is being imported circularly before its defined. Which happens because imports are currently happening directly from the modules

@RXminuS RXminuS closed this Oct 1, 2019
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.

2 participants