Skip to content

diff-npm-package: fix reports dir creation#3624

Merged
IvanGoncharov merged 1 commit intographql:mainfrom
yaacovCR:fixer
Jun 2, 2022
Merged

diff-npm-package: fix reports dir creation#3624
IvanGoncharov merged 1 commit intographql:mainfrom
yaacovCR:fixer

Conversation

@yaacovCR
Copy link
Copy Markdown
Contributor

@yaacovCR yaacovCR commented Jun 1, 2022

#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.

@yaacovCR
diff-npm-package: polish reports dir creation
ae54286 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 1, 2022 •
edited
Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit ae54286
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6297ac980647870008f3a053
😎 Deploy Preview https://deploy-preview-3624--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR
Copy link
Copy Markdown
Contributor Author

yaacovCR commented Jun 1, 2022

History:

  1. I noticed that there was a problem with pull requests in terms of creating the report file: See, for example: https://github.com/graphql/graphql-js/runs/6670599072?check_suite_focus=true#step:6:12.
  2. I surmised this was because of the recent PR diff-npm-package: move report into shared 'reports' folder #3611 changing the reports directory.
  3. I assumed that the problem was that the directory was not being created, so I submitted diff-npm-package: create reports folder #3623 to try to fix that.
  4. @IvanGoncharov noticed that there was something wrong with the check, and @saihaj submitted a fix for it, and diff-npm-package: create reports folder #3623 got merged.
  5. But, the truth is, that the report was not being created at all because no diff was found, so the check I added should never have been hit.
  6. I am unclear why the former PRs found a diff, but with the creation of diff-npm-package: create reports folder #3623, no diff was found. I find this very strange.
  7. Anyway, I am submitting what I hope is a better fix to diff-npm-package: create reports folder #3623, as the logic there unfortunately was still off, as the "reports" directory was being created with the same path later used as the report filename -- there needs to be two different paths, one for the creation of the folder, one for the creation of the file.
  8. I am not sure how to test this, as the report is not being created, and I am not sure how to actually force the report to be created.

@yaacovCR
Copy link
Copy Markdown
Contributor Author

yaacovCR commented Jun 1, 2022

I am not sure why, but luckily #3622 still triggers a diff, and is still failing, so can be used to test this PR.

After rebasing, as expected, now mkdir is failing because we are calling it with the wrong path. Merging this PR should fix that, and so at least after we merge this, we can use that PR to see if the change works.

At that point, CI from #3622 should give us the diff, and we might be able to see why there is one!

@yaacovCR yaacovCR requested a review from a team June 2, 2022 10:19
@yaacovCR yaacovCR changed the title diff-npm-package: polish reports dir creation diff-npm-package: fix reports dir creation Jun 2, 2022
@IvanGoncharov
Copy link
Copy Markdown
Member

IvanGoncharov commented Jun 2, 2022

Thanks for the detailed, investigation 👍
Your description gives a lot of contexts.

I am not sure how to actually force the report to be created.

Any change that affects the content of the NPM file (add stuff to package.json, LICENSE, README, or any files under 'src/').

@IvanGoncharov
Copy link
Copy Markdown
Member

IvanGoncharov commented Jun 2, 2022

Context: I need this script to see what changed inside the NPM package.
It's relevant on PRs that update tsconfig.json, resources/build-npm.js, etc.
It was super useful during TS migration.
Before that, I was doing the same by cloning PRs and generating before/after builds and running diff on it.
For example, it would be relevant in upcoming CJS/MJS discussions.

@IvanGoncharov IvanGoncharov merged commit 540bb38 into graphql:main Jun 2, 2022
@yaacovCR yaacovCR mentioned this pull request Jun 2, 2022
Closed
@yaacovCR yaacovCR deleted the fixer branch June 10, 2022 13:16
@jasonkuhrt jasonkuhrt mentioned this pull request Oct 29, 2024
Merged
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
896894e 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
e2a6df8 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
7c489bd 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
2cbec20 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
cd27d1c 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 16, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
6f08492 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
c0a0acf 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
8fe2170 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 22, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
4978453 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Dec 22, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
82de238 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Dec 22, 2025
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
0d3ecba 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jan 27, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (graphql#3624)
b4f57e1 
graphql#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 22, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
1d90774 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 23, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
3331410 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 24, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
f245ffb 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 24, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
3319c0d 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 24, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
9bb7158 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
yaacovCR added a commit that referenced this pull request Feb 24, 2026
@yaacovCR
diff-npm-package: polish reports dir creation (#3624)
d724ca0 
#3623 creates the reports folder via the report path, rather than the reports folder path. This did not trigger an error, because no changes were found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

PR: internal 🏠

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yaacovCR @IvanGoncharov