Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/empty-horses-relate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphql-eslint/eslint-plugin': patch
---

fix false positive cases for `require-import-fragment` on Windows, when `graphql-config`'s `documents` key contained glob pattern => source file path of document contained always forward slashes
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#import 'bar-fragment.gql'
#import './fragments/bar-fragment.gql'
query {
foo {
...FooFields
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#import FooFields from "bar-fragment.gql"
#import FooFields from "./fragments/bar-fragment.gql"
query {
foo {
...FooFields
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Imports could have extra whitespace and double/single quotes

# import 'foo-fragment.gql'
# import './fragments/foo-fragment.gql'

query {
foo {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Imports could have extra whitespace and double/single quotes

# import FooFields from "foo-fragment.gql"
# import FooFields from "./fragments/foo-fragment.gql"

query {
foo {
Expand Down
11 changes: 8 additions & 3 deletions packages/plugin/src/documents.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { resolve } from 'node:path';
import path from 'node:path';
import debugFactory from 'debug';
import fg from 'fast-glob';
import { GraphQLProjectConfig } from 'graphql-config';
Expand All @@ -15,13 +15,18 @@ const handleVirtualPath = (documents: Source[]): Source[] => {
return documents.map(source => {
const location = source.location!;
if (['.gql', '.graphql'].some(extension => location.endsWith(extension))) {
return source;
return {
...source,
// When using glob pattern e.g. `**/*.gql` location contains always forward slashes even on
// Windows
location: path.resolve(location),
};
Comment on lines -18 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavain-sundaysky this is better fix, so other rules will get always os specific slash

}
filepathMap[location] ??= -1;
const index = (filepathMap[location] += 1);
return {
...source,
location: resolve(location, `${index}_document.graphql`),
location: path.resolve(location, `${index}_document.graphql`),
};
});
};
Expand Down
35 changes: 20 additions & 15 deletions packages/plugin/src/rules/require-import-fragment/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,55 +1,60 @@
import { join } from 'node:path';
import { CWD } from '@/utils.js';
import path from 'node:path';
import { ParserOptionsForTests, ruleTester } from '../../../__tests__/test-utils.js';
import { rule } from './index.js';

function withMocks({ name, filename, errors }: { name: string; filename: string; errors?: any }) {
function withMocks({
name,
filename,
errors,
only = false,
}: {
name: string;
filename: string;
errors?: any;
only?: boolean;
}) {
return {
name,
filename,
code: ruleTester.fromMockFile(filename.split('mocks')[1]),
parserOptions: {
graphQLConfig: {
documents: [
filename,
join(CWD, '__tests__', 'mocks', 'import-fragments', 'foo-fragment.gql'),
join(CWD, '__tests__', 'mocks', 'import-fragments', 'bar-fragment.gql'),
],
documents: [filename, './__tests__/mocks/import-fragments/fragments/**/*.gql'],
},
} satisfies ParserOptionsForTests,
errors,
only,
};
}

ruleTester.run('require-import-fragment', rule, {
valid: [
withMocks({
name: 'should not report with named import',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'valid-query.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'valid-query.gql'),
}),
withMocks({
name: 'should not report with default import',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'valid-query-default.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'valid-query-default.gql'),
}),
withMocks({
name: 'should not report fragments from the same file',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'same-file.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'same-file.gql'),
}),
],
invalid: [
withMocks({
name: 'should report with named import',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'invalid-query.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'invalid-query.gql'),
errors: [{ message: 'Expected "FooFields" fragment to be imported.' }],
}),
withMocks({
name: 'should report with default import',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'invalid-query-default.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'invalid-query-default.gql'),
errors: [{ message: 'Expected "FooFields" fragment to be imported.' }],
}),
withMocks({
name: 'should report fragments when there are no import expressions',
filename: join(CWD, '__tests__', 'mocks', 'import-fragments', 'missing-import.gql'),
filename: path.resolve('__tests__', 'mocks', 'import-fragments', 'missing-import.gql'),
errors: [{ message: 'Expected "FooFields" fragment to be imported.' }],
}),
],
Expand Down
12 changes: 6 additions & 6 deletions packages/plugin/src/rules/require-import-fragment/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'node:path';
import { NameNode } from 'graphql';
import { GraphQLESTreeNode } from '../../estree-converter/index.js';
import { GraphQLESLintRule } from '../../types.js';
import { requireSiblingsOperations } from '../../utils.js';
import { requireSiblingsOperations, slash } from '../../utils.js';

const RULE_ID = 'require-import-fragment';
const SUGGESTION_ID = 'add-import-expression';
Expand Down Expand Up @@ -89,21 +89,21 @@ export const rule: GraphQLESLintRule = {

const extractedImportPath = comment.value.match(/(["'])((?:\1|.)*?)\1/)?.[2];
if (!extractedImportPath) continue;

const importPath = path.join(path.dirname(filePath), extractedImportPath);
const importPath = path.join(filePath, '..', extractedImportPath);
const hasInSiblings = fragmentsFromSiblings.some(
source => source.filePath === importPath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously you added path.resolve(source.filePath) but why?

source.filePath comes from graphql-config and contains slashes according to user's os

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so too, but when I printed out the filePath in fragmentsFromSiblings by adding

console.log(fragmentsFromSiblings.map(x=>x.filePath));

I saw this syntax. On Windows.

C:/Dev/my-project/client/logic/experiments/experimentFragment.graphql

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange because I added console log on ci and get backslash on windows

Copy link
Contributor

@dimaMachina dimaMachina Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here I commented d9f5b4e and got

IMG_2887

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder maybe it happens when you specify glob syntax for documents? e.g. **/*.gql

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was right, specifying glob pattern returns forward slash always

image

);
if (hasInSiblings) return;
}

const fragmentInSameFile = fragmentsFromSiblings.some(
source => source.filePath === filePath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, source.filePath comes from graphql-config and contains slashes according to user's OS, and filePath is ESLint's context.filename which also contains slashed according user's OS

);
if (fragmentInSameFile) return;

const suggestedFilePaths = fragmentsFromSiblings.length
? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath))
? fragmentsFromSiblings.map(o =>
// Use always forward slash for suggested import path
slash(path.relative(path.dirname(filePath), o.filePath)),
)
: ['CHANGE_ME.graphql'];

context.report({
Expand Down
14 changes: 7 additions & 7 deletions packages/plugin/src/rules/require-import-fragment/snapshot.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ exports[`require-import-fragment > invalid > should report fragments when there

#### 💡 Suggestion: Add import expression for "FooFields".

1 | # import FooFields from 'foo-fragment.gql'
1 | # import FooFields from 'fragments/foo-fragment.gql'
2 | {
3 | foo {
4 | ...FooFields
Expand All @@ -29,7 +29,7 @@ exports[`require-import-fragment > invalid > should report fragments when there
exports[`require-import-fragment > invalid > should report with default import 1`] = `
#### ⌨️ Code

1 | #import 'bar-fragment.gql'
1 | #import './fragments/bar-fragment.gql'
2 | query {
3 | foo {
4 | ...FooFields
Expand All @@ -45,8 +45,8 @@ exports[`require-import-fragment > invalid > should report with default import 1

#### 💡 Suggestion: Add import expression for "FooFields".

1 | # import FooFields from 'foo-fragment.gql'
2 | #import 'bar-fragment.gql'
1 | # import FooFields from 'fragments/foo-fragment.gql'
2 | #import './fragments/bar-fragment.gql'
3 | query {
4 | foo {
5 | ...FooFields
Expand All @@ -57,7 +57,7 @@ exports[`require-import-fragment > invalid > should report with default import 1
exports[`require-import-fragment > invalid > should report with named import 1`] = `
#### ⌨️ Code

1 | #import FooFields from "bar-fragment.gql"
1 | #import FooFields from "./fragments/bar-fragment.gql"
2 | query {
3 | foo {
4 | ...FooFields
Expand All @@ -73,8 +73,8 @@ exports[`require-import-fragment > invalid > should report with named import 1`]

#### 💡 Suggestion: Add import expression for "FooFields".

1 | # import FooFields from 'foo-fragment.gql'
2 | #import FooFields from "bar-fragment.gql"
1 | # import FooFields from 'fragments/foo-fragment.gql'
2 | #import FooFields from "./fragments/bar-fragment.gql"
3 | query {
4 | foo {
5 | ...FooFields
Expand Down
3 changes: 1 addition & 2 deletions packages/plugin/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ export const logger = {
export const slash = (path: string): string => path.replaceAll('\\', '/');

// Match slash or backslash for Windows
// eslint-disable-next-line no-useless-escape
export const VIRTUAL_DOCUMENT_REGEX = /[\/\\]\d+_document.graphql$/;
export const VIRTUAL_DOCUMENT_REGEX = /[/\\]\d+_document.graphql$/;

export const CWD = process.cwd();

Expand Down