Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve generation performance #8642

Conversation

jantimon
Copy link
Contributor

@jantimon jantimon commented Nov 24, 2022

Description

graphql-codegen-cli/src/generate-and-save.ts has more filesystem than neccessary:

  1. Checking for existence before reading. (from the official nodejs docs):

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile() or fs.writeFile() is not recommended.

  1. After reading the file from disk and generating the hash, the hash is not stored.
    So if the previousHash matches the currentHash the file will be read again from disk.

  2. mkdirp is now built into node

This PR improves all three cases and in addition fixes a bug where input.cwd is not passed to mkdirp.

How Has This Been Tested?

All unit tests + tested on my machine - would love to get feedback from a windows user.

Test Environment:

  • OS: MacOs 13
  • @graphql-codegen/...:
  • NodeJS: 16

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@changeset-bot
Copy link

changeset-bot bot commented Nov 24, 2022

🦋 Changeset detected

Latest commit: a27b71f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@graphql-codegen/cli Patch
@graphql-cli/codegen Patch

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

@charlypoly
Copy link
Contributor

@ardatan I put you in review to QA on Windows 👀 🪟

@saihaj saihaj merged commit 5afa923 into dotansimha:master Nov 24, 2022
@saihaj
Copy link
Collaborator

saihaj commented Nov 25, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants