Skip to content

Commit

Permalink
[🔥AUDIT🔥] Fix up some more alias resolution in graphql-flow. (#70)
Browse files Browse the repository at this point in the history
🖍 _This is an audit!_ 🖍

## Summary:
Missed some other edge cases from my previous PR, this should fix that!

Issue: FEI-5745

## Test plan:
I updated the graphql-flow dep in the static service with a link: and confirmed that this new command works as expected.

Author: jeresig

Auditors: #frontend-infra-web

Required Reviewers:

Approved By:

Checks: ✅ Lint & Test (ubuntu-latest, 20.x)

Pull Request URL: #70
  • Loading branch information
jeresig authored Jul 30, 2024
1 parent b0ea7e8 commit 45f09ef
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 40 deletions.
5 changes: 5 additions & 0 deletions .changeset/forty-pens-sip.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/graphql-flow": patch
---

Fix up some more alias resolution in graphql-flow.
20 changes: 18 additions & 2 deletions schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,23 @@
"generate": {"oneOf": [
{"$ref": "#/definitions/GenerateConfig"},
{"type": "array", "items": {"$ref": "#/definitions/GenerateConfig"}}
]}
]},
"alias": {
"type": "array",
"items": {
"type": "object",
"additionalProperties": false,
"properties": {
"find": {
"type": ["string", "object"]
},
"replacement": {
"type": "string"
}
},
"required": [ "find", "replacement" ]
}
}
},
"required": [ "crawl", "generate" ]
}
}
20 changes: 5 additions & 15 deletions src/cli/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {GraphQLSchema} from "graphql/type/schema";
import {generateTypeFiles, processPragmas} from "../generateTypeFiles";
import {processFiles} from "../parser/parse";
import {resolveDocuments} from "../parser/resolve";
import {getPathWithExtension} from "../parser/utils";
import {findApplicableConfig, getSchemas, loadConfigFile} from "./config";

import {addTypenameToDocument} from "apollo-utilities";
Expand Down Expand Up @@ -81,22 +82,11 @@ const inputFiles = cliFiles.length
/** Step (2) */

const files = processFiles(inputFiles, config, (f) => {
if (existsSync(f)) {
return readFileSync(f, "utf8");
const resolvedPath = getPathWithExtension(f, config);
if (!resolvedPath) {
throw new Error(`Unable to find ${f}`);
}
if (existsSync(f + ".js")) {
return {text: readFileSync(f + ".js", "utf8"), resolvedPath: f + ".js"};
}
if (existsSync(f + ".ts")) {
return {text: readFileSync(f + ".ts", "utf8"), resolvedPath: f + ".ts"};
}
if (existsSync(f + ".tsx")) {
return {
text: readFileSync(f + ".tsx", "utf8"),
resolvedPath: f + ".tsx",
};
}
throw new Error(`Unable to find ${f}`);
return {text: readFileSync(resolvedPath, "utf8"), resolvedPath};
});

let filesHadErrors = false;
Expand Down
4 changes: 2 additions & 2 deletions src/parser/__test__/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ describe("getPathWithExtension", () => {
expect(result).toBe("/path/to/file.js");
});

it("returns an empty string if no file is found", () => {
it("returns null if no file is found", () => {
// Arrange
jest.spyOn(fs, "existsSync").mockImplementation((path) => false);

// Act
const result = getPathWithExtension("/path/to/file", config);

// Assert
expect(result).toBe("");
expect(result).toBe(null);
});

it("maps aliases to their correct value", () => {
Expand Down
15 changes: 9 additions & 6 deletions src/parser/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import traverse from "@babel/traverse";

import path from "path";

import {getPathWithExtension} from "./utils";
import {fixPathResolution, getPathWithExtension} from "./utils";
import {Config} from "../types";

/**
Expand Down Expand Up @@ -152,6 +152,7 @@ export const processFile = (
text: string;
resolvedPath: string;
},
config: Config,
): FileResult => {
const dir = path.dirname(filePath);
const result: FileResult = {
Expand All @@ -177,7 +178,7 @@ export const processFile = (

ast.program.body.forEach((toplevel) => {
if (toplevel.type === "ImportDeclaration") {
const newLocals = getLocals(dir, toplevel, filePath);
const newLocals = getLocals(dir, toplevel, filePath, config);
if (newLocals) {
Object.keys(newLocals).forEach((k) => {
const local = newLocals[k];
Expand Down Expand Up @@ -386,6 +387,7 @@ const getLocals = (
dir: string,
toplevel: ImportDeclaration,
myPath: string,
config: Config,
):
| {
[key: string]: Import;
Expand All @@ -395,9 +397,10 @@ const getLocals = (
if (toplevel.importKind === "type") {
return null;
}
const importPath = toplevel.source.value.startsWith(".")
? path.resolve(path.join(dir, toplevel.source.value))
: toplevel.source.value;
const fixedPath = fixPathResolution(toplevel.source.value, config);
const importPath = fixedPath.startsWith(".")
? path.resolve(path.join(dir, fixedPath))
: fixedPath;
const locals: Record<string, any> = {};
toplevel.specifiers.forEach((spec) => {
if (spec.type === "ImportDefaultSpecifier") {
Expand Down Expand Up @@ -439,7 +442,7 @@ export const processFiles = (
if (!next || files[next]) {
continue;
}
const result = processFile(next, getFileSource(next));
const result = processFile(next, getFileSource(next), config);
files[next] = result;
listExternalReferences(result, config).forEach((path) => {
if (!files[path] && !toProcess.includes(path)) {
Expand Down
5 changes: 4 additions & 1 deletion src/parser/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ const resolveImport = (
},
config: Config,
): Document | null | undefined => {
const absPath: string = getPathWithExtension(expr.path, config);
const absPath = getPathWithExtension(expr.path, config);
if (!absPath) {
return null;
}
if (seen[absPath]) {
errors.push({
loc: expr.loc,
Expand Down
25 changes: 11 additions & 14 deletions src/parser/utils.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import fs from "fs";
import {Config} from "../types";

export const getPathWithExtension = (
pathWithoutExtension: string,
config: Config,
): string => {
// Process the path so that we can handle aliases.
export const fixPathResolution = (path: string, config: Config) => {
if (config.alias) {
for (const {find, replacement} of config.alias) {
pathWithoutExtension = pathWithoutExtension.replace(
find,
replacement,
);
path = path.replace(find, replacement);
}
}
return path;
};

export const getPathWithExtension = (
pathWithoutExtension: string,
config: Config,
) => {
pathWithoutExtension = fixPathResolution(pathWithoutExtension, config);
if (
/\.(less|css|png|gif|jpg|jpeg|js|jsx|ts|tsx|mjs)$/.test(
pathWithoutExtension,
Expand All @@ -33,9 +34,5 @@ export const getPathWithExtension = (
if (fs.existsSync(pathWithoutExtension + ".ts")) {
return pathWithoutExtension + ".ts";
}
// NOTE(john): This is a bit of a hack, but it's necessary for when we
// have a file that doesn't exist. This will happen when we delete all of
// the type files before re-running graphql-flow again. We want to ensure
// that we don't error out in this case.
return "";
return null;
};

0 comments on commit 45f09ef

Please sign in to comment.