Feat/file extension

[adrienharnay]

TODO:

• Update CHANGELOG.md* with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

@JakeDawkins Hey, I have done the initial work and started trying to test it but I'm not sure what's the best way to do it, would you have some insights for me? Thanks!

Fixes #1004

[trevor-scheer]

This is looking pretty good @adrienharnay! Just a couple things that should help tie it together and make sure it's doing exactly what we want.

You'll need to add your flag to the Generate class, which represents the CLI's codegen command:
https://github.com/apollographql/apollo-tooling/blob/master/packages/apollo/src/commands/client/codegen.ts#L30

A great way to test your work would be to add a couple tests here, and run npm test to see if they pass. You should be able to follow the pattern that's laid out there, but if you need any help please feel free to ask!
Codegen tests for reference:
https://github.com/apollographql/apollo-tooling/blob/master/packages/apollo/src/commands/client/__tests__/generate.test.ts

[adrienharnay]

Thank you for your guidance! I will work on this today and ping you when I'm done 🙂

[JakeDawkins]

Thanks for jumping in @trevor-scheer :)

[adrienharnay]

Hey @trevor-scheer sorry for the delay, I have added a description for the flag in the Generate class, and started looking at the tests, but it seems to me that the tests that look the most like what I would write are currently skipped: https://github.com/apollographql/apollo-tooling/blob/master/packages/apollo/src/commands/client/__tests__/generate.test.ts#L381. Do you have any idea why, and do I need to know anything about it before writing the tests? Cheers 🙂

[JakeDawkins]

@adrienharnay sorry for the delay, we're all really busy right now, but we really appreciate your work here!

Some of the tests in that file are broken from old migrations, but most of the tests in that file still work. You should be able to duplicate this test with the addition of your flag to test it out.

[adrienharnay]

No worries! Alright thank you, I will follow your instructions 👍

[adrienharnay]

@JakeDawkins I wrote the test but realized that my new flag doesn't appear in context.options (in generateSource). I don't understand why, would you mind taking a look when you have time please? Thanks!

[JakeDawkins]

@adrienharnay I added a couple questions/notes to the PR. I also figured out your problem with not passing the flag, but I can't add a comment to that line since it's further down in the file.

In commands/client/codegen.ts when the generate function is called, you have to manually pass the flags through to the last parameter of that function.

if you add fileExtension: flags.fileExtension to line 200 that should fix it!

[adrienharnay]

@JakeDawkins Agreed with every of your comments! I have updated the PR, the tests pass, I have one last error:

client:codegen › writes typescript types for query with client-side data when client schema in graphql file

    ENOENT: no such file or directory, mkdir '../__tmp__Wt33yg/outDirectory'

It seems like my test causes another test to fail 🧐

Edit: forgot to thank you for the fix, thanks!

[adrienharnay]

@JakeDawkins Hey, I was away last week. I still can't understand why writing a file at the default location caused another test to fail... Could you please take a look?

[adrienharnay]

@JakeDawkins Sorry to bother you again, could you take a look when you're available? Thanks :)

[JakeDawkins]

@adrienharnay Hey! Sorry, I've been really bad with keeping up with this. I don't really have the time to figure out why the error is happening. It's something to do with the FS setup we're doing, but it looks a little tricky from what I can see.

If you add the --outputFlat option to the CLI command in the test, it should fix it though.

[adrienharnay]

@JakeDawkins Hey, no worries about this! Your advice indeed did the trick, and everything is green locally. Some tests seem to fail on the Azure Pipelines though, not really sure what this is about...

[JakeDawkins]

Ah, yeah the Azure errors is just an error that randomly appears. It's been around for a few months now 😞 Rerunning usually passes those. As for the Generated Types Check failures, those are just old types that have been updated in engine since running last. You can probably fix this by either rebasing off master or running npx run client:codegen

[JakeDawkins]

Ooh I stand corrected. It looks like there was more than one Azure error 🤔

[adrienharnay]

Generated Types Check is good, but yeah Azure seems grumpy... Some tests are passing on Linux but not Windows? Including some I didn't touch 😅

[andymrussell]

Is there any progress on this pr?

[adrienharnay]

Hey, I would like to ship this but can't understand why the tests don't pass :(

[JakeDawkins]

@adrienharnay so sorry this has been on deck for so long! I can try to get this in ASAP.

It looks like the builds in azure were too old, so I can't find them. And triggering them manually isn't seeming to work. Can you push an empty commit
git commit --allow-empty to trigger a build?

[adrienharnay]

Done 🙂

[JakeDawkins]

@adrienharnay ah, I think I see a problem. When logging out the files in ./outDirectory, I see 2 files, one for simpleQuery and the other for globalTypes.

Since we're pulling off the first item in the list of files as filePath, I suppose windows is pulling off the global types file instead of the simpleQuery file. So we probably need to just check and make sure we're using the correct file path.

Also, I'm not 100% sure why the file is ending up with a . before the extension

[JakeDawkins]

Sorry, one last nitpick @adrienharnay! Can you change the flag to tsFileExtension instead of fileExtension? I don't think we want to put in the work to support this across all languages since it's not really relevant elsewhere.

Other than that, I think this can be merged quickly :)

comment

[JakeDawkins]

Released with apollo@2.18.0! Thanks for your persistence and patience @adrienharnay :)

[adrienharnay]

Ah, sorry I was on bank holliday week-end! Thanks for your help on this feature!

[JakeDawkins]

No worries! I had some spare time 😁