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

Implement from schema file feature #247

Merged
merged 16 commits into from
Dec 16, 2019

Conversation

jouderianjr
Copy link
Contributor

Hey Dillon, as we discussed some months ago in my company we used to generate the elm files based on a shared schema file, not from the introspection file. So, this pr adds schema-file options.

I can add in the future a feature to fetch the file from a remote server.

@jouderianjr
Copy link
Contributor Author

I'm getting a weird error regarding the graphql dependency. It is only working if I set the NODE_ENV to production of if I disable the minification from parcel. I will take a look at how to solve it

Error:

Error: Cannot use e "__Schema" from another module or realm.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.

@jouderianjr
Copy link
Contributor Author

@dillonkearns I don't think there is a way to fix this because it seems an error related to parcel minification.

So, we have 2 options, remove the minification temporary or wait for some fix from them. what do you think about it?

@dillonkearns
Copy link
Owner

Hey! I'm all for removing the minification. The only thing is that it uses Typescript, and it imports Elm code. So we need some sort of build process. Would it work to compile it in dev mode but still use parcel? Otherwise I imagine it would be a lot of work to set up a streamlined build process.

Thanks for working on this!

@jouderianjr
Copy link
Contributor Author

@dillonkearns yeah, parcel has an argument to remove the minification --no-minify, I pushed a commit adding this to the pr as you agree about removing the minification.

I have to thank you to create such a useful library :D

@dillonkearns
Copy link
Owner

Nice, glad that solved it!

Another thought (for a future request) is that it might be nice to remove those flags entirely and just have the argument infer what to do with it from context. For example, if it starts with http(s), then treat it like a url. If not, check for a file. If the file is valid JSON, then parse it as a schema JSON dump. If not, then try to treat the file contents as a GraphQL SDL schema file. And if anything fails, print a message explaining the different things you can provide and how it handles them. Anyway, just a thought for how to simplify the UX a bit here for another iteration of the CLI.

Thanks for contributing this! I'll just check it out and make sure everything looks good before pushing merging this in. Thank you!

@jouderianjr
Copy link
Contributor Author

Yeah, I think it make sense make it more user-friendly, I will give a try next week to play with it, as we are using in my company, I will be glad to help to improve it :D

@dillonkearns
Copy link
Owner

Hello @jouderianjr, that would be great, let me know if you need any help on that change!

I pushed some code to move things to 0.19.1 since there were some build issues around that, and I've got it to a green build now (sorry for the git diff noise). I'll merge this in and ship it. Thanks again for your contribution! 🙏

@dillonkearns dillonkearns merged commit 1c89a0d into dillonkearns:master Dec 16, 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