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-source-filesystem): add optional name parameter to createRemoteFileNode #11054

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

oorestisime
Copy link
Contributor

Fix: #11037

Description

Take an optional name parameter to fix issue with temporary urls having no "guessable" name

Related Issues

@sidharthachatterjee
Copy link
Contributor

Thank you for this, @oorestisime

Would you like to also add a test or two to assert this behaviour and ensure we don't see a regression again? 🙂

@oorestisime
Copy link
Contributor Author

Thank you for this, @oorestisime

Would you like to also add a test or two to assert this behaviour and ensure we don't see a regression again? slightly_smiling_face

I would really like to do so but for this particular package it is always hard, at least for me. There are a lot of mocks to do and basically i am not sure how do to do so (plus there are not many in place to get inspired). If you have an example in mind i can work on it probably.

@sidharthachatterjee
Copy link
Contributor

@oorestisime So in packages/gatsby-source-filesystem/src/__tests__/create-remote-file-node.js you could add a test asserting overrides name if passed in

In this you would pass in a name and a url and assert that createNode got called with the right one

Check out the documentation at https://jestjs.io/docs/en/mock-functions for how to listen on arguments a mock fn is called with

@oorestisime
Copy link
Contributor Author

But until now these tests were not entering here https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-filesystem/src/create-remote-file-node.js#L180-L262

and there i need to mock requestRemoteNode etc. what if i change getRemoteFileName to take as parameters a url and optionnaly a name. so that i could indeed unit-test only that part?

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

@sidharthachatterjee it's fairly challenging to test this in a clean way. We can mock--but we need to mock quite a bit, and then those test(s) will likely become a little fragile and will require a fair amount of tweaking when we make changes to this functionality.

It would be excellent if we can perhaps refactor this a bit to enable easier unit testing (and cleaner code!), but I don't think that should be a blocker to merging this PR. That being said--subequent PRs addressing some of the things we're talking about here would be 👌

Thanks to both of you!

@DSchau DSchau changed the title Add optional name parameter to create remote file node fix(gatsby-source-filesystem): add optional name parameter to createRemoteFileNode Jan 23, 2019
@DSchau DSchau changed the title fix(gatsby-source-filesystem): add optional name parameter to createRemoteFileNode feat(gatsby-source-filesystem): add optional name parameter to createRemoteFileNode Jan 23, 2019
@DSchau DSchau merged commit 8105be6 into gatsbyjs:master Jan 23, 2019
@DSchau
Copy link
Contributor

DSchau commented Jan 23, 2019

Successfully published:
 - [email protected]

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
…RemoteFileNode (gatsbyjs#11054)

Fix: gatsbyjs#11037

<!--
  Have any questions? Check out the contributing docs at https://gatsby.app/contribute, or
  ask in this Pull Request and a Gatsby maintainer will be happy to help :)
-->

## Description

Take an optional name parameter to fix issue with temporary urls having no "guessable" name
<!-- Write a brief description of the changes introduced by this PR -->

## Related Issues

<!--
  Link to the issue that is fixed by this PR (if there is one)
  e.g. Fixes gatsbyjs#1234, Addresses gatsbyjs#1234, Related to gatsbyjs#1234, etc.
-->
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.

[Regression] createRemoteFileNode is not able to guess filename from temporary urls
3 participants