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

feat(gatsby-page-utils): extract logic for watching a directory from gatsby-page-creator so can reuse for custom page creation #14051

Merged
merged 16 commits into from
Jun 21, 2019

Conversation

frinyvonnick
Copy link
Contributor

@frinyvonnick frinyvonnick commented May 15, 2019

Description

It extract logic from gatsby-plugin-page-creator to help creating pages:

  • ignorePath
  • validatePath
  • createPath
  • watchDirectory

Related Issues

Closes #13606

@KyleAMathews
Copy link
Contributor

KyleAMathews commented May 17, 2019

Can you add to the description some sample code showing how someone would use this in their gatsby-node.js?

@pieh
Copy link
Contributor

pieh commented May 17, 2019

Should gatsby-plugin-page-creator make use of those extracted utilities to not duplicate code?

@KyleAMathews
Copy link
Contributor

Yeah, what @pieh said. That should be part of this PR.

@frinyvonnick frinyvonnick changed the title feat(gatsby-page-helper): extract logic for watching a directory from gatsby-page-creator so can reuse for custom page creation feat(gatsby-page-utils): extract logic for watching a directory from gatsby-page-creator so can reuse for custom page creation May 20, 2019
@frinyvonnick
Copy link
Contributor Author

frinyvonnick commented May 24, 2019

Sure I'll add it as soon as I figure out how to extract watching directory functionnality. I have difficulties to know to limit of things to extract. What function signature did you have in mind @KyleAMathews ?

Maybe someone could help on this part by pairing ?

@frinyvonnick
Copy link
Contributor Author

Maybe someone from @gatsbyjs/core could answer my question about function signature of the watching directory functionnality ? 😄

@freiksenet
Copy link
Contributor

@frinyvonnick Sorry for dropping the ball here!

The API could look something like that:

watchDirectory(
  path: String;
  glob: String;
  onNewFile: (newPath) => void;
  onRemovedFile: (removedPath) => void;
): Promise<undefined>

It should track which files were already created and which were not.

@freiksenet freiksenet added the status: awaiting author response Additional information has been requested from the author label Jun 10, 2019
@frinyvonnick
Copy link
Contributor Author

Thank you @freiksenet, I'll go with it 👌!

@frinyvonnick frinyvonnick requested a review from a team as a code owner June 11, 2019 11:30
@frinyvonnick
Copy link
Contributor Author

@KyleAMathews I added some documentation and an example in the README.md. Is this good for you ? Should I add documentation somewhere else ?

@freiksenet I implemented the signature you suggested. Seems good to you ?

@gatsbyjs/core can't figure out how to make e2e passes ? Could someone help me on this subject ?

@wardpeet
Copy link
Contributor

mmh let me look into this properly

@frinyvonnick frinyvonnick added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author status: WIP labels Jun 18, 2019
@wardpeet
Copy link
Contributor

Fingers crossed 🤞.

gatsby-dev didn't copy the plugin as it was a name mismatch between package.json & foldername. Also the watchDirectory export wasn't a default export but I changed all exports to commonjs (require) to be consistent.

@frinyvonnick
Copy link
Contributor Author

Yeah it works 🎉 thanks for your help @wardpeet! Sorry for the typo 😕

Copy link
Contributor

@muescha muescha left a comment

Choose a reason for hiding this comment

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

just my suggestions and nitpicks

packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-page-utils/README.md Outdated Show resolved Hide resolved
packages/gatsby-plugin-page-creator/src/gatsby-node.js Outdated Show resolved Hide resolved
frinyvonnick and others added 4 commits June 19, 2019 08:37
Co-Authored-By: Michael <184316+muescha@users.noreply.github.com>
Co-Authored-By: Michael <184316+muescha@users.noreply.github.com>
Co-Authored-By: Michael <184316+muescha@users.noreply.github.com>
@frinyvonnick
Copy link
Contributor Author

Thank you for your review @muescha 👍

@wardpeet
Copy link
Contributor

Yeah it works 🎉 thanks for your help @wardpeet! Sorry for the typo 😕

No need to say sorry, it happens to the best of us 😉

Thanks for doing this!

@frinyvonnick
Copy link
Contributor Author

@wardpeet this is ready to be merged, no ? 😄

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Oh yeah definitely, the code looks great.

@wardpeet wardpeet merged commit 68d9d6f into gatsbyjs:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract logic for watching a directory from gatsby-page-creator so can reuse for custom page creation
6 participants