Allow gatsby-link to be used in other contexts like Storybook #1611
Conversation
|
I didn't see any docs or tests that would need to be updated for this change, but if I missed a good place to add a test, let me know. |
|
Deploy preview ready! Built with commit ae91ff6 |
|
Deploy preview ready! Built with commit ae91ff6 |
|
Deploy preview ready! Built with commit ae91ff6 |
packages/gatsby-link/src/index.js
Outdated
| @@ -3,7 +3,7 @@ import { Link, NavLink } from "react-router-dom" | |||
| import PropTypes from "prop-types" | |||
|
|
|||
| let pathPrefix = `` | |||
| if (__PREFIX_PATHS__) { | |||
| if (typeof __PREFIX_PATHS__ !== "undefined" && __PREFIX_PATHS__) { | |||
There was a problem hiding this comment.
I think it would be better to use lodash for this check as it reads better and the code can be more consistent.
import { isUndefined } from 'lodash'
_.isUndefined(__PREFIX_PATHS__)
There was a problem hiding this comment.
@Alaev lodash isUndefined can catch if a variable is declared and set to undefined, but it will throw a reference error like any other function call if it is passed a variable name that has not been declared at all. The only things I know of that can handle completely undeclared variable names are typeof or a try-catch block.
There was a problem hiding this comment.
I agree Lodash is generally nicer but as gatsby-link is used on every Gatsby site out there, we want to add as little code as possible so the "raw" check in this PR is better.
There was a problem hiding this comment.
or use it as this:
const isUndefined = require('lodash.isUndefined');
const _ = require('lodash');
There was a problem hiding this comment.
I agree that it would look nicer though :(
There was a problem hiding this comment.
I see I will try to dig on this one for a bit tomorrow.
There was a problem hiding this comment.
@benmccormick quick one, should there be a check for null?
There was a problem hiding this comment.
undefined is correct.
The null value represents an object with a non-existent reference. undefined is a clearer primitive, indicating that no value has been assigned at all.
|
The second half of the statement checks if the variable is falsy. That
encompasses undefined, null, false, 0 and ''. So that should be ok
…On Tuesday, July 25, 2017, Michael Alaev ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/gatsby-link/src/index.js
<#1611 (comment)>:
> @@ -3,7 +3,7 @@ import { Link, NavLink } from "react-router-dom"
import PropTypes from "prop-types"
let pathPrefix = ``
-if (__PREFIX_PATHS__) {
+if (typeof __PREFIX_PATHS__ !== "undefined" && __PREFIX_PATHS__) {
@benmccormick <https://github.com/benmccormick> quick one, should there
be a check for null?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1611 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABz9pTlSWhIclLnFebvTYg0I-3UmaDwqks5sRl1XgaJpZM4OjDz8>
.
|
|
@KyleAMathews just to clarify, are we having this wait on #1581 landing? So the list looks like this?
|
|
Yup! And just merged #1581 so step 1 is done :-) |
benmccormick
force-pushed
the
gatsby-link
branch
2 times, most recently
from
52d4303 to
51047fd
Compare
|
@KyleAMathews and now step 2 is done :) . Test added and verified that it previously failed and now passes |
| @@ -19,6 +19,12 @@ const getNavigateTo = () => { | |||
| } | |||
|
|
|||
| describe(`<Link />`, () => { | |||
|
|
|||
| it(`does not fail to initialize when __PREFIX_PATHS__ is not defined`, () => { | |||
There was a problem hiding this comment.
Another way you could write this (I think!) is like this
expect(() => {
const Link = require(`../`).default
const link = new Link({})
}).not.toThrow();So that you're actually writing a test and not relying on side effects or anything. Also probably want to drop the eslint comments from your commit!
There was a problem hiding this comment.
@DSchau yeah that probably is more clear. Jest fails tests on uncaught exceptions, so its ok either way, but I'll look at that.
As for the eslint comments... eslint was warning about them in my editor so I added them. ¯_(ツ)_/¯. Any reason I shouldn't care?
There was a problem hiding this comment.
@DSchau updated the test. It does give nicer feedback when the test fails in that format. Good suggestion.
There was a problem hiding this comment.
As for the eslint comments
Only because (as far as I remember), I haven't seen any in the existing code-base, and it's pretty typically advisable to kinda keep that stuff as-is unless for a good reason! However I'll defer to @KyleAMathews on that one!
benmccormick
force-pushed
the
gatsby-link
branch
from
51047fd to
ae91ff6
Compare
|
Awesome, thanks! |
This afternoon I was experimenting with Storybook to view my Gatsby components and test different states, as preparation for some redesign work on my blog.
Unfortunately loading in components that imported
gatsby-linkfailed in that context since__PREFIX_PATH__was undefined. There are workarounds to this, but there doesn't seem to be any reason that gatsby-link should require that variable to be defined to be used (since it doesn't require it to actually have a value, just a defined reference) so this PR adds atypeofcheck, which will allow it to be imported safely into environments like Storybook.