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

[gatsby-remark-copy-linked-files] Fix bad copy location with route dir and improve tests #2875

Merged

Conversation

Undistraction
Copy link
Contributor

The ongoing saga of adding destinationDir option to gatsby-copy-linked-files The third in a trilogy, but unlike Robocop 3, this is the best of the bunch.

  • Fixes another issue with invalid path.
  • Adds more and better tests.

@KyleAMathews Using the gatsby-dev-cli has been really helpful. Not sure how I missed it before, but I've noticed that all logging within plugins (when run in an app/site) is suppressed. Is there a way to enable logging in plugins when running them? I mean that logging works fine when testing within the Gatsby project, but when I pull the plugin into a project that uses Gatsby, all logging is gone. Even if I directly add to the derived file in node_modules.

@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 10, 2017

Deploy preview ready!

Built with commit 861224c

https://deploy-preview-2875--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 10, 2017

Deploy preview ready!

Built with commit 861224c

https://deploy-preview-2875--using-drupal.netlify.com

@Undistraction Undistraction changed the title Fix bad copy location with route dir and improve tests [gatsby-remark-copy-linked-files] Fix bad copy location with route dir and improve tests Nov 10, 2017
@alampros
Copy link
Contributor

🥇 for PR description of the day

@KyleAMathews
Copy link
Contributor

Suppressed how? I use console.log all the time in plugins during development.

@KyleAMathews
Copy link
Contributor

The tests are failing on Windows https://ci.appveyor.com/project/KyleAMathews/gatsby/build/1.0.3942/job/39cjovk10p1emxcn

I think you need to use normalize-path c.f. #2658 (comment) to ensure paths are the same in all OSes.

@KyleAMathews
Copy link
Contributor

Struggling to find the issue for some reason but I wonder if this is related to a bug @bvaughn found on reactjs.org where public was repeated dozens of times in a generated path.

@Undistraction
Copy link
Contributor Author

There were a couple of path.join in the test, so I have replaced with path.posix.join.

@Undistraction
Copy link
Contributor Author

Struggling to find the issue for some reason but I wonder if this is related to a bug @bvaughn found on reactjs.org where public was repeated dozens of times in a generated path.

I actually fixed a bug like this - it was caused by passing the actual opts object into the plugin (instead of a copy). The opts object was mutated, and the mutated object became the new opts object. I was going to suggest that you pass in copies to the plugins as this is much safer.

@KyleAMathews
Copy link
Contributor

There were a couple of path.join in the test, so I have replaced with path.posix.join.

No path.join is correct — you just need to use normalize-path. If you read the comment I linked to, using path.posix.join was the cause of previous windows breakage.

@KyleAMathews
Copy link
Contributor

Oh weird… yeah I guess we shouldn't trust plugins to not mutate options :-(

Want to add a PR to clone the options here?

@Undistraction
Copy link
Contributor Author

Looks like the tests for gatsby-remark-copy-linked-files are passing on Travis, but some other unrelated tests are now failing?

Want to add a PR to clone the options here?

Sure.

@Undistraction
Copy link
Contributor Author

I'm confused - the comment you link to says:

The problem was paths being joined using path.join rather than path.posix.join

And the plugin already used posix.join in multiple locations so doesn't it make sense to use posix.join?

And the failing tests are not tests for gatsby-remark-copy-linked-files. I don't see how changes I've made to a plugin could be causing others to fail.

Sorry if I'm being dense. Long day.

@KyleAMathews
Copy link
Contributor

Oh hmm sorry — I'm being dense :-) Yeah you're right — since posix.join is fine in this case as you're just generating a path for the test from hard coded values — not from paths you read from the OS which would vary by OS. Ok, merging this — there are some other windows test failures which have crept in which I'll look into.

@KyleAMathews KyleAMathews merged commit f01ba7e into gatsbyjs:master Nov 10, 2017
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.

4 participants