-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
refactor(gatsby): remove parsePath from gatsby core, and move to gatsby-link #9957
refactor(gatsby): remove parsePath from gatsby core, and move to gatsby-link #9957
Conversation
Moving a utility function (that was not used!) in gatsby core into where it belongs, gatsby-link. This was discovered via a community member (@travi), as he was attempting to use storybook to test a Link component (outside of Gatsby), e.g. `import Link from 'gatsby-link'`, which was then requiring `gatsby` core. This also generally solves some weirdness that the cyclical require was kinda weird, e.g. gatsby -> gatsby-link -> gatsby I do not believe this is a breaking change, since the version required of gatsby-link will just be bumped by gatsby, and the export is still exported from the top level gatsby module
It is used in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/navigation.js (but we could import it from |
i've been heads down on a few things, so i've lost track of whats happening with this one. is this still planned to move forward somehow? anything i should try to help out with to help it move forward? |
Fixed! I do think this change is probably worth merging in, because it's more correct and resolves some issues for folks. I think it also resolves a circular dependency which is a nice little side benefit, e.g. gatsby -> gatsby-link -> gatsby becomes gatsby -> gatsby-link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @DSchau 👍
Released in
|
thank you for pushing this through. it does appear to have resolved my issue. one thing that i noticed that seems like it may have been unintentional is that the display-name changed from |
I don't think this was introduced in this PR. We added Also for context, is this in the React Devtools? I think that's perhaps sort of of a known issue a la https://spectrum.chat/styled-components/help/styled-components-v4-react-devtools-bloat~a5856708-0786-499d-ab32-3626b45a1094 |
very possible. i've been distracted with other projects lately, so i havent given this a lot of attention lately.
in my case (at least for now), it wasn't. my tests were using enzyme to select the link for inspection in the test assertions using the display-name approach for that selection. i changed the tests to select using the variable approach instead, so the change isn't very impactful for me. however, it was the use of devtools that came to mind with mentioning it being less descriptive since i do also inspect the react tree sometimes in that way. just seemed like it would be less obvious in that case that it was a component even related to gatsby-link. |
Moving a utility function (that was not used!) in gatsby core into where it belongs, gatsby-link. This was discovered via a community member (@travi), as he was attempting to use storybook to test a Link component (outside of Gatsby), e.g.
import Link from 'gatsby-link'
, which was then requiringgatsby
core.This also generally solves some weirdness that the cyclical require was kinda weird, e.g. gatsby -> gatsby-link -> gatsby
I do not believe this is a breaking change, since the version required of gatsby-link will just be bumped by gatsby, and the export is still exported from the top level gatsby module