Initial query extraction using AST #553

evans · 2018-08-14T22:05:35Z

This is a query extraction technique. We currently output map from sha to query string in a file, manifest.json. The output location could be customizable and probably should be stdout by default, so that a user can pipe it into a file should they choose.

fixes #556

abernix

This is great! Super glad to see the foundation is concise and straightforward. Just a couple comments...

abernix · 2018-08-15T07:25:37Z

packages/apollo-cli/src/commands/queries/extract.ts

+import { Command, flags } from '@oclif/command';
+import * as Listr from 'listr';
+import { print } from 'graphql';
+import { sha512 } from 'js-sha512';


Was there a particular reason for using js-sha512, a JavaScript implementation of the SHA-512 routine, rather than Node.js’ native crypto.createHash (which is implemented in C/C++).

abernix · 2018-08-15T07:26:30Z

packages/apollo-cli/src/commands/queries/extract.ts

@@ -0,0 +1,80 @@
+import 'apollo-codegen-core/lib/polyfills';


What’s this polyfill relatated to? Might be worth putting a comment about its necessity/purpose since it’s imported in its entirety rather generically.

I'm not sure why it's included myself. I'll dig around and see. My first guess is lack of native support for the functions in Node 8

abernix · 2018-08-15T07:28:38Z

packages/apollo-cli/src/commands/queries/extract.ts

+import { sha512 } from 'js-sha512';
+import * as fs from 'fs';
+
+import { loadQueryDocuments } from 'apollo-codegen-core/lib/loading';


Longer term, is it confusing to leave the loadQueryDocuments functionality in apollo-codegen-core? (It doesn’t really sound like a utility library and we’re not doing code-gen.)

hmm we could probably make the judgement of apollo-codegen-core in general. Let's keep it in mind as we continue digging into the functionality

danilobuerger · 2018-08-16T07:40:44Z

packages/apollo-cli/src/commands/queries/extract.ts

+      {
+        title: 'Outputing manifest',
+        task: async ctx => {
+          fs.writeFileSync('manifest.json', JSON.stringify(ctx.mapping));


This should really be an option before this gets merged. People will riot otherwise 😂

😆 Totally! What do you think about the default being output to stdout and then having an option to specify the file with --output? Or should it automatically send it to the manifest?

That would be one option, but consider the rest of the cli that just takes the output path as the last arg without a flag.

evans · 2018-08-29T06:30:31Z

This can be tested with a command similar to

(cd ~/engine/frontend \
&& ~/apollo-cli/packages/apollo-cli/bin/run queries:extract \
    --queries=**/*.jsx --queries=./imports/**/*.js \
&& cat manifest.json | jq)

abernix · 2018-08-29T07:42:24Z

packages/apollo-codegen-core/src/loading.ts

+    matches = extractDocumentsWithAST(content, options);
+  } catch (e) {
+    console.log('Extraction using AST failed with \n', e, '\nRetrying using regex');
+    const re = new RegExp(tagName + 's*`([^`]*)`', 'g');


Curious what the s* here is meant to implement? Tag plurality? (Is that a thing?; i.e. gqls, graphqls?)

We should look into why the parse is failing since it seems to necessitate a switch from the underlying parser to something which supports more modern ECMAScript features — a configuration tweak, usually.

All in all, I'm opposed to falling back to a regular expression parse technique in the event that an AST parse has failed. While it will work in some cases, it's still fragile and could mis-identify or under-match an operation.

For example, this regular expression won't support a properly-escaped backtick within a document, and it will fail to properly concatenate two operations which have been joined together (e.g. with +, Array.prototype.join, etc.).

Looks like gqls is not a thing prettier/prettier#2092. I wasn't sure about it and hadn't pinned down the usage. Let's remove it!

Removing the regex fallback makes sense. It will cause some configuration pain in some cases., i.e. actively excluding .d.ts files, since there are many that don't parse well. For the most part, I trust the parsers. Running it on spectrum and the engine frontend yielded good results.

abernix · 2018-08-29T13:35:20Z

packages/apollo-cli/src/commands/queries/check.ts

-import { format } from "../schema/check";
-import { resolveDocumentSets } from "../../config";
-import { loadConfigStep } from "../../load-config";
+import { engineFlags } from '../../engine-cli';


Would it be possible to revert the Prettier changes present in 7214296? We should run it on the whole repository outside the scope of this PR if that's what we want, but for the sake of accurately reviewing this PR it should be kept as a separate concern.

evans · 2018-08-29T23:11:48Z

The behavior of the query:extract now mirrors schema:download in terms of output location.

The PR also improves fragment inclusion for query checking, fixing #556. The changes also apply to mobile, since the fragment inclusions work for .gql and .graphql files.

Outputs map from sha to query string in a file, manifest.json. This could be customizable and really probably should be piped to std out by default, so that a user can pipe it into a file should they choose. * [squash] fix hash digest * remove unused code left from query:check * use native node crypto library * query:extract remove unused json flag

This approach becomes much too fragile with dynamic imports, so a different technique is used in a later commit.

Also output the sha in base64

Fallback on the naive regext search for gql` in the cases that the parsing fails. This also removes the code that chased down imports and attempted to resolve them with variable/import analysis. This strategy proves to be too fragile when attempting to construct an interpolated string. There are too many different ways to import values. Additionally in the future world of import(), which returns a promise, would be impossible to support. * import apollo-engine-reporting for AST manipulation * remove unused variables * continue traversing AST for nested template literal

Updates the @bable/parser version. Improves error message for parser failure. * add clarity to parser choosing logic

We add to the Error's message from parser instead of throwing our own error

This moves the extraction and combination to apollo-codegen-core. These operations are shared across other operations, such as type generation.

In loading.ts and extract.ts

This matches the queries:download arguments

This isolates the test cases that are failing and indicates that the file globing should be done in a single location.

The tests depend on the fact that the git remote is set to apollo-cli

loadConfigStep pulls the queries value from the flags passed to the command, so there is no need to use a custom parsing function. Though the name of the function requires an in-depth look at the implementation.

Dogfoods apollo queries:check

evans · 2018-09-04T21:15:14Z

Comments from @benjamn to optimize the solution:

use a traversal that does not construct NodePath objects, since we don’t need to know about the ancestry of the node
use a regular expression to find possible gql identifiers, and then ignore subtrees that don’t contain any
(harder) modify the parser so that just calls a callback when it hits a gql-tagged template string, and doesn’t generate AST nodes we don’t care about

Hopefully these are useful in the future 🤞

evans requested a review from abernix August 14, 2018 22:05

abernix reviewed Aug 15, 2018

View reviewed changes

danilobuerger reviewed Aug 16, 2018

View reviewed changes

evans force-pushed the evans/query-extraction branch 2 times, most recently from 7b7d144 to 63c76f2 Compare August 29, 2018 05:48

abernix reviewed Aug 29, 2018

View reviewed changes

evans force-pushed the evans/query-extraction branch 3 times, most recently from a99c0aa to d45e43c Compare August 29, 2018 22:38

evans force-pushed the evans/query-extraction branch from 27e9209 to ea03f70 Compare August 30, 2018 17:08

evans changed the title ~~[WIP] Initial query extraction~~ Initial query extraction using AST Aug 31, 2018

evans force-pushed the evans/query-extraction branch from a72d3dc to 0d1c0b6 Compare August 31, 2018 02:44

evans force-pushed the evans/query-extraction branch 2 times, most recently from fcf239f to 15d1d97 Compare August 31, 2018 17:17

evans added 13 commits August 31, 2018 11:09

Make query extration import aware

e972c4d

This approach becomes much too fragile with dynamic imports, so a different technique is used in a later commit.

Add fragments to operations during extraction

be07ae8

Also output the sha in base64

Use specific parser for each file type

238de06

Updates the @bable/parser version. Improves error message for parser failure. * add clarity to parser choosing logic

Remove regex fallback

b6b6de5

We add to the Error's message from parser instead of throwing our own error

Move operation and fragment manipulation to core

02407d8

This moves the extraction and combination to apollo-codegen-core. These operations are shared across other operations, such as type generation.

Reuse the extraction code for query checks

804555f

Include polyfills for flatMap

3798812

In loading.ts and extract.ts

Add code to deal with nested fragments

591a2c3

Add output argument to queries:extract

ffd24aa

This matches the queries:download arguments

Move glob resolution for queries flag to commands

ede6ac1

This isolates the test cases that are failing and indicates that the file globing should be done in a single location.

Add note about nock debugging

4e64d19

Comment about test dependency on git remote

f634bfe

The tests depend on the fact that the git remote is set to apollo-cli

Fix tests using loadConfigStep to parse flags

23abc81

loadConfigStep pulls the queries value from the flags passed to the command, so there is no need to use a custom parsing function. Though the name of the function requires an in-depth look at the implementation.

evans force-pushed the evans/query-extraction branch 6 times, most recently from 3bd807f to b0f4a74 Compare August 31, 2018 18:57

Add query check to circle ci

53b2724

Dogfoods apollo queries:check

evans force-pushed the evans/query-extraction branch from b0f4a74 to 53b2724 Compare August 31, 2018 19:04

evans merged commit ebae069 into master Aug 31, 2018

evans deleted the evans/query-extraction branch August 31, 2018 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial query extraction using AST #553

Initial query extraction using AST #553

evans commented Aug 14, 2018 •

edited

Loading

abernix left a comment

abernix Aug 15, 2018

abernix Aug 15, 2018

evans Aug 15, 2018

abernix Aug 15, 2018

evans Aug 15, 2018

danilobuerger Aug 16, 2018

evans Aug 16, 2018

danilobuerger Aug 17, 2018

evans commented Aug 29, 2018 •

edited

Loading

abernix Aug 29, 2018

abernix Aug 29, 2018 •

edited

Loading

evans Aug 29, 2018

evans Aug 29, 2018

abernix Aug 29, 2018

evans commented Aug 29, 2018

evans commented Sep 4, 2018

Initial query extraction using AST #553

Initial query extraction using AST #553

Conversation

evans commented Aug 14, 2018 • edited Loading

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

abernix Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans commented Aug 29, 2018

evans commented Sep 4, 2018

evans commented Aug 14, 2018 •

edited

Loading

evans commented Aug 29, 2018 •

edited

Loading

abernix Aug 29, 2018 •

edited

Loading