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

Restore ability to put generated files in a directory relative to source #470

Merged
merged 4 commits into from
Jul 5, 2018
Merged

Conversation

mike-marcacci
Copy link
Contributor

Fixes #469. Before the change from apollo-codegen to apollo cli, flow type definitions were output into a __generated__ directory, next to their respective source files.

This adds a localDirectory flag that accepts a name to use locally for generated types. For example:

apollo generate:flow **/*.graphql --localDirectory=__generated__

This flag is ignored if --output is specified.

@ghost ghost added blocking 🎉 feature New addition or enhancement to existing solutions labels Jul 2, 2018
@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Jul 2, 2018

Please note that the format changes are the result of a fixed precommit hook, which was incorrectly defined and failing; this was updated and the formatter was applied to the to-be-changed files in the first commit.

It appears that the precommit script was incorrectly defined in the packages/apollo-cli/package.json file, which was making it impossible to commit without a --no-verify flag.

I fixed the configuration, and applied the format changes to the to-be-changed files in this commit.
Before the change from `apollo-codegen` to `apollo`, flow type definitions were output into a `__generated__` directory, next to their respective source files. The new `localDirectory` flag restores support for this kind of workflow by accepting a custom directory name:

```sh
apollo codegen:generate --target=flow --localDirectory=__generated__
```

This flag is ignored if `--output` is specified.
@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Jul 2, 2018

Please also note that something in your test on travis fails nondeterministically. I force-pushed the exact same code a couple times with amended commits (separating out the formatting changes from the meaningful ones) and sometimes it would fail (with messages about using a custom engine key), other times it would succeed. This (obviously) has nothing to do with my PR, but I figured you should be aware.

@martijnwalraven
Copy link
Contributor

I definitely think this behavior is worth supporting, maybe even as a default. But if we want to keep it under an option, I'm wondering if we can find a more descriptive alternative to --localDirectory. Are there any examples of other CLIs that have flags for similar options?

@mike-marcacci
Copy link
Contributor Author

Hmm, personally my preference would be to reuse the --output flag, and add a boolean --output-relative-to-source but I was looking to make as few ripples as possible (just to get it working again, since maintaining forks of a monorepo is a big nuisance).

Would that be a workable solution, and default to --output=__generated__ --output-relative-to-source for Flow/TypeScript generators? I think this would be my ideal solution.

@martijnwalraven
Copy link
Contributor

@mike-marcacci That makes sense to me, but curious to hear what @shadaj and @jbaxleyiii think.

@mike-marcacci
Copy link
Contributor Author

Placing Flow and TypeScript files in directories relative to their source files is now the default behavior. By default, the directory name used is __generated__, which can be overridden by setting the output (first) cli arg.

To use the output to specify a single locaation the new --outputRelativeToCWD flag can be set.

To continue placing generated files immediately adjacent their source files (which is the current behavior, albiet only for a couple days and arguably undesireable) output can be set to an empty string. While available, I suspect this feature will never be used, due to the problems outlined in #468 and #469.


Side note: for consistency with the other flags, I used outputRelativeToCWD instead of output-relative-to-source since dash separations are part of oclif boolean flags, such as the no- prefix but apollo CLI had chosen to use a lowerCamelCase convention. I don't really care either way, but I'm curious why this switch was made, since apollo-codegen had used dash separations up until now.

Placing Flow and TypeScript files in directories relative to their source files is now the default behavior. By default, the directory name used is `__generated__`, which can be overridden by setting the `output` (first) cli arg.

To use the `output` to specify a single locaation the new `--outputRelativeToCWD` flag can be set.

To continue placing generated files immediately adjacent their source files (which is the current behavior, albiet only for a couple days and arguably undesireable) `output` can be set to an empty string. While available, I suspect this feature will never be used, due to the problems outlined in #468 and #469.
@mike-marcacci
Copy link
Contributor Author

Also, to reiterate my comment about problems with the travis setup, the difference between these builds is a newline in the commit message:

@shadaj
Copy link
Contributor

shadaj commented Jul 3, 2018

Hi @mike-marcacci this looks great! outputRelativeToCWD seems a bit hard to understand for me, would something like outputFlat work to express the idea of not placing types next to sources?

@mike-marcacci
Copy link
Contributor Author

@shadaj good idea. The PR's been updated with your suggestion.

@mike-marcacci mike-marcacci changed the title Add localDirectory flag to TypeScript/Flow generators. Restore ability to put generated files in a directory relative to source Jul 4, 2018
@ghost ghost added blocking 🎉 feature New addition or enhancement to existing solutions labels Jul 4, 2018
@jbaxleyiii jbaxleyiii merged commit 5e407ff into apollographql:master Jul 5, 2018
@shadaj shadaj added the 🐞 bug label Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug 🎉 feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants