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

Add ignoreIndex option to fileLoader #176

Closed
wants to merge 1 commit into from

Conversation

iminside
Copy link

@iminside iminside commented Oct 2, 2018

Hi guys!
I open this PR to add option ignoreIndex for fileLoader. For backward compatibility this option is true by default. I also added tests and changed the readme.
#135

@RichardLitt RichardLitt added the 🚀enhancement New feature or request label Oct 2, 2018
Copy link
Contributor

@cfnelson cfnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! 🙌
This looks like a pretty straight fwd change. 👍
I think we should be fine to accept this changes. cc @RodMachado Thoughts?

Out of curiosity is this a feature/functionality that you would like to use/are using yourself? Would you be able to provide an example file/folder structure of what this looks like?

Would it be possible to update the description of the tests to be a little more descriptive of what is occurring in the test.

e.g we use this terminology everywhere loads all files from specified folder, but with this change it is even more misleading. As we are not loading all files, we are ignoring index files, and your new test is actually loading all files from the specified folder including the index files.

If it's possible I think it would be great to see an example test of this fileLoader option being used alongside mergeTypes/mergeResolvers as the behaviour may be unexpected to some people if they mix the other options with this ignoreIndex option. I have concerns about normal index files being loaded and then passed to the merge functions (might not be an issue but the thought cropped up).

Or maybe at least it would be good to add a word of warning in the readme about using ignoreIndex option and mixing it with the other options that change behaviour.

@iminside
Copy link
Author

iminside commented Oct 3, 2018

I use this functionality along with glob ./*/index.ts i.e. fileLoader(‘./*/index.ts’, {ignoreIndex: false}).
Sample structure of project:

/schema
/user
——user.gql // graphql schema
——user.ts // user resolvers
——index.ts // export user resolvers
—index.ts // export all resolvers
main.ts // start server
// schema/user/user.ts

import { IUserResolvers, IQueryMeResolver } from 'core'

export const User: IUserResolvers = {
    id: parent => parent.id,
    name: parent => parent.name
}

export const Query = {
    me: <IQueryMeResolver>((parent, args, ctx) => {
        ...
    })
}
// schema/user/index.ts
import * as resolvers from './user'
export default resolvers
// schema/index.ts
import { mergeTypes, mergeResolvers, fileLoader } from 'merge-graphql-schemas'
import { IResolvers } from 'core'

export const typeDefs = mergeTypes(fileLoader('./**/*.gql'), { all: true })
export const resolvers = mergeResolvers<IResolvers>(fileLoader('./*/index.ts',  {ignoreIndex: false}))
// main.ts
import { GraphQLServer } from 'graphql-yoga'
import { typeDefs, resolvers } from './schema'

const server = new GraphQLServer({
    typeDefs,
    resolvers
} as any)

server.start(() => console.log('Server is running on localhost:4000'))

we are ignoring index files, and your new test is actually loading all files from the specified folder including the index files.

No, old tests work as before - ignore index files. Option ignoreIndex default is true.
I created only one new test for check loading files when ignoreIndex: false.

Or maybe at least it would be good to add a word of warning in the readme about using ignoreIndex option and mixing it with the other options that change behaviour.

Yes indeed, in cases other than this fileLoader('./*/index.ts', {ignoreIndex: false}), this option may be unpredictable for many people. I added it to load the resolvers from the usual index files and discard the naming of user.resolvers.ts.
Maybe add to readme something like: "Use this option only if you really know what you are doing."?

@cfnelson
Copy link
Contributor

cfnelson commented Oct 7, 2018

Thanks for the detailed explanation. I know am wondering why you need or want to load the index files with fileLoader if you are already exporting/importing the types & resolvers to the index file.

My main worry/concern is that it may be unpredictable and cause subtle bugs or issues. The fileLoader is currently the main issue people have due to they way they use it along with their webpack setup. So I am cautious about adding any additional points of confusion. If we were to accept this PR we would like a mention in the Readme to use this with caution etc. and to only use if they are confident in it capturing the correct files. e.g -> doesn't pick up unrelated index files.

cc @RodMachado Would like your review on this PR please. 😃

@cerinoligutom
Copy link

Any updates on this?

ardatan added a commit that referenced this pull request Jul 15, 2019
@ardatan
Copy link
Collaborator

ardatan commented Jul 15, 2019

Merged, thanks!

@ardatan ardatan closed this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants