Move gatsby-link into gatsby #3864
Conversation
This change is mostly copying the files from `gatsby-link` package to `gatsby` package. Have added a missing dependency (prop-types) and changed the `main` file to `dist/index.js` in gatsby's package.json.
Just copying over index.d.ts from `gatsby-link` package to `gatsby` and removing default export as we don't export `Link` component as the default one.
ghost
assigned 
packages/gatsby/.babelrc
Outdated
| } | ||
| ] | ||
| ] | ||
| } |
There was a problem hiding this comment.
this is correct, you shouldn't need the babelrc here, the existing setup will handle it correctly when it's imported in the browser code.
packages/gatsby/src/index.js
Outdated
| @@ -0,0 +1,3 @@ | |||
| import Link, { withPrefix, navigateTo } from './components/link' | |||
|
|
|||
| export { Link, withPrefix, navigateTo } | |||
There was a problem hiding this comment.
I'm not sure this is the best way to handle the imports here...I think we want an explicit browser entry file, so that all the server side code isn't accidently (in the future) imported.
I would make a browser.js or maybe client.js (w/d/t @KyleAMathews) and point to that file in package.json browser: main field (more info)
There was a problem hiding this comment.
cool, never realized that browser field existed in package.json 😄 Thanks for pointing out. WIll probably wait for @KyleAMathews' comment and make browser.js or client.js.
| @@ -0,0 +1,189 @@ | |||
| /*global __PREFIX_PATHS__, __PATH_PREFIX__ */ | |||
| import React from "react" | |||
There was a problem hiding this comment.
Rather then moving the source code here it might make sense to leave the external pacakge and have gatsby depend on it directly and re-export
There was a problem hiding this comment.
Hmm...quite not sure about this. If there are plans to have more components in gatsby package, I think it's better to have the code here as we may not want other components to be in their own packages. If that's not something we're looking at now, yeah, it makes sense to just leave the package as is and re-export the component.
There was a problem hiding this comment.
it might be the case taht new components live here, but I do think there is probably less friction to re-export. The other benefit tho is that someone can update/bug fix gatsby-link without requiring a gatsby release. That said I defer to what @KyleAMathews thinks
There was a problem hiding this comment.
I agree with @jquense. It's a much smaller change to leave it where it is and just export it. Also it keeps our packaging setup simpler as with gatsby-link we know it's browser code and we can build and publish it accordingly.
There was a problem hiding this comment.
The other benefit tho is that someone can update/bug fix gatsby-link without requiring a gatsby release.
@jquense, might be a silly question, but I don't quite understand this - do you mean we'll keep publishing new changes to gatsby-link on to npm as a separate package without publishing gatsby? If we want Link to be imported from gatsby, we've to discontinue publishing a separate package no?
There was a problem hiding this comment.
@tsriram no we'd still publish gatsby-link as a separate package — it'd just now be a dependency of gatsby and we'd instruct people to use the re-exported version there.
@jquense lerna would actually still publish gatsby on every new gatsby-link change — excess of caution perhaps but whatever. We already release gatsby a few times a day or so so no big deal :-)
There was a problem hiding this comment.
@KyleAMathews Cool. gatsby-link already seems to be a dependency of gatsby:
gatsby/packages/gatsby/package.json
Line 52 in fb6313e
So only re-exporting needs to be done here I guess.
There was a problem hiding this comment.
Also updating example sites and www and any docs referencing gatsby-link :-)
existing babel setup should already handle this!
|
Looks great! Merging this now. Would love help as well updating example sites + docs :-) |
|
Sure, I was about to comment that this was WIP and to be merged after I update example sites and docs but it was too late last night. Will send a new PR for that :) |
|
I got an itchy merge finger :-) |
This is for #3714.
Just copied over files from
gatsby-linktogatsby, added required dependencies and changed themainfile todist/index.jsingatsby's package.json. Testing importing Link fromgatsbyin a local project and it seems to be working fine.Wonder if I should delete
gatsby-linkpackage already?