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.

Sign up for GitHub

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

Allow gatsby-link to be used in other contexts like Storybook #1611

Merged
merged 1 commit into from
Aug 1, 2017
+10 −1
Merged

Allow gatsby-link to be used in other contexts like Storybook #1611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/gatsby-link/src/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ const getNavigateTo = () => {
}

describe(`<Link />`, () => {

it(`does not fail to initialize when __PREFIX_PATHS__ is not defined`, () => {
Copy link
Contributor

@DSchau DSchau Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DSchau updated the test. It does give nicer feedback when the test fails in that format. Good suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

expect(() => {
const Link = require(`../`).default
const link = new Link({}) //eslint-disable-line no-unused-vars
}).not.toThrow()
})

describe(`path prefixing`, () => {
it(`does not include path prefix by default`, () => {
const to = `/path`
Expand Down
3 changes: 2 additions & 1 deletion packages/gatsby-link/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/*global __PREFIX_PATHS__, __PATH_PREFIX__ */
import React from "react"
import { Link, NavLink } from "react-router-dom"
import PropTypes from "prop-types"

let pathPrefix = ``
if (__PREFIX_PATHS__) {
if (typeof __PREFIX_PATHS__ !== `undefined` && __PREFIX_PATHS__) {
pathPrefix = __PATH_PREFIX__
}

Expand Down