Fix Issue #2276 #2277
Fix Issue #2276 #2277
Conversation
🦋 Changeset detectedLatest commit: 59d7783 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 |
|
Anyone can review and merge this? |
|
Hey @dotansimha, |
There was a problem hiding this comment.
Hi @yoavain-sundaysky, I just set up running Windows on CI and fixed some issues with Windows. Could you add a test with your change?
* Convert `source.filePath` to its absolute path to be comparable to importPath/filePath on Windows
* Convert `source.filePath` to its absolute path to be comparable to importPath/filePath on Windows
yoavain-sundaysky
force-pushed
the
fix-2276-require-import-fragment-on-windows
branch
from
ebea6a5 to
fabf3c3
Compare
Sure, I'll do that |
…th delimiter types in input
…n-windows Add require-import-fragment test for relative path on Windows with both delimiter types in input
|
Added tests. |
| ); | ||
|
|
||
| const hasInSiblings = fragmentsFromSiblings.some( | ||
| source => source.filePath === importPath, |
There was a problem hiding this comment.
previously you added path.resolve(source.filePath) but why?
source.filePath comes from graphql-config and contains slashes according to user's os
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Strange because I added console log on ci and get backslash on windows
There was a problem hiding this comment.
here I commented d9f5b4e and got
There was a problem hiding this comment.
I wonder maybe it happens when you specify glob syntax for documents? e.g. **/*.gql
There was a problem hiding this comment.
yeah, I was right, specifying glob pattern returns forward slash always
| } | ||
|
|
||
| const fragmentInSameFile = fragmentsFromSiblings.some( | ||
| source => source.filePath === filePath, |
There was a problem hiding this comment.
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
| return source; | ||
| return { | ||
| ...source, | ||
| // When using glob pattern e.g. `**/*.gql` location contains always forward slashes even on | ||
| // Windows | ||
| location: path.resolve(location), | ||
| }; |
There was a problem hiding this comment.
@yoavain-sundaysky this is better fix, so other rules will get always os specific slash
4 participants
Fix Issue #2276
source.filePathto its absolute path to be comparable to importPath/filePath on Windows🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant
motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-eslint/...:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...