Add CLI option to generate-persisted-query-manifest that lists all matched files using the documents pattern
#344
Conversation
🦋 Changeset detectedLatest commit: 831ebe8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| generatePersistedQueryManifest, | ||
| getFilepaths, | ||
| defaults, | ||
| } = require("./dist/index.js"); |
There was a problem hiding this comment.
Not new in this PR, but I think this dist/ is probably` unnecessary? I don't see any other uses of it in the repo.
There was a problem hiding this comment.
I'm blaming you for that 😜. Though I probably should have improved this when I took over haha.
But ya, we could probably do something here to make this a tad better. Using dist here means that I have to rebuild the package anytime I make changes (including for tests), which is a real pain. I'll see if I can address this in separate PR.
There was a problem hiding this comment.
I mean I think you can just delete it and this might have been some sort of VSCode-added mistake?
There was a problem hiding this comment.
Perhaps I'm misunderstanding your ask here. All of these functions are used in this file, so I need to import them from somewhere (here is getFilepaths in cli.js for example). If I update this to be require('./index.js'), the CLI ceases to function with the error:
node:internal/modules/cjs/loader:1051
throw err;
^
Error: Cannot find module './index.js'
Require stack:
- /Users/jerelmiller/code/apollo-utils/packages/generate-persisted-query-manifest/cli.js
at Module._resolveFilename (node:internal/modules/cjs/loader:1048:15)
at Module._load (node:internal/modules/cjs/loader:901:27)
at Module.require (node:internal/modules/cjs/loader:1115:19)
at require (node:internal/modules/helpers:119:18)
at Object.<anonymous> (/Users/jerelmiller/code/apollo-utils/packages/generate-persisted-query-manifest/cli.js:9:5)
at Module._compile (node:internal/modules/cjs/loader:1233:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
at Module.load (node:internal/modules/cjs/loader:1091:32)
at Module._load (node:internal/modules/cjs/loader:938:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/Users/jerelmiller/code/apollo-utils/packages/generate-persisted-query-manifest/cli.js'
]
}
Node.js v20.4.0
Am I missing something obvious that you're asking here?
There was a problem hiding this comment.
OH. I missed that this is in fact a very special file — it's a JS file run directly, not a TS file. I can't recall why I decided to do that (as you point out, it is my fault) though I suppose the file was somewhat smaller back then :)
It would be reasonable to see if we could switch away from having any JS files in the repo, or getting it back down to being basically one line long — but certainly not in this PR.
There was a problem hiding this comment.
Most definitely! I figured you probably knew something I didn't when you started the CLI, especially since I don't have a ton of experience building them, so I've left it as a .js file. I'll explore migrating this to a ts file :)
| @@ -176,6 +176,10 @@ function uniq<T>(arr: T[]) { | |||
| return [...new Set(arr)]; | |||
| } | |||
|
|
|||
| export async function getFilepaths(documents: string | string[]) { | |||
| return uniq(await globby(documents)); | |||
There was a problem hiding this comment.
Not new in this PR but I am curious if getFilepaths returns the paths in sorted (or deterministic at all) order?
My guess is that the answer is that uniq preserves ordering (because Sets do), and that globby probably gives you a deterministic ordering that is based on the initial set of roots you pass in and then sorted for things discovered for each root.
Or maybe not. This old ish issue suggests that globby order is nondeterministic: sindresorhus/globby#131
Perhaps this function should sort? Determinism sounds like a nice feature.
There was a problem hiding this comment.
Ya that is a good callout. I thought about this as well. The only thing is that sort gets a bit gnarly if directory depth is important in sorting. For example, this test applied with a generic .sort() ends up with this order:
[
'src/components/legacy.js',
'src/components/my-component.tsx',
'src/queries/root.graphql',
'src/query.graphql'
]Notice how the query.graphql file is sorted later, which is higher in the directory tree. I opted to leave this as-is for now for this reason, though you're right, it is not currently deterministic.
Let me see if I can tinker with this a bit more to see if I can come up with something that maintains the directory hierarchy while adding some determinism without making it too gnarly.
There was a problem hiding this comment.
I changed my mind on this. Listing strictly by alphabetical order on a large codebase looks more correct to me than trying to add hierarchical sorting so I'll just use that. Updated in 831ebe8
jerelmiller
force-pushed
the
option-for-all-files
branch
from
223c87f
to
0187cc7
Compare
Adds a
--list-filesoption to the CLI that will print the list of files matched against the configureddocumentspattern. This can be useful for debugging why the command may or may not be including/not including specific queries in the manifest.