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

Jump to bottom

[v2] fix find-page #5228

Merged
merged 2 commits into from
May 3, 2018
Merged

[v2] fix find-page #5228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81ba2c2
[v2] fix find-page
nihgwu May 2, 2018
772587c
fix test
nihgwu May 2, 2018
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: 4 additions & 4 deletions packages/gatsby/cache-dir/__tests__/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import loader from "../loader.js"

describe(`Loader`, () => {
beforeEach(() => {
delete global.__PATH_PREFIX__
delete global.__PREFIX_PATHS__
global.__PATH_PREFIX__ = ``
global.__PREFIX_PATHS__ = false

// Workaround for Node 6 issue: https://github.com/facebook/jest/issues/5159
if (global.hasOwnProperty(`__PATH_PREFIX__`))
global.__PATH_PREFIX__ = undefined
global.__PATH_PREFIX__ = ``
if (global.hasOwnProperty(`__PREFIX_PATHS__`))
global.__PREFIX_PATHS__ = undefined
global.__PREFIX_PATHS__ = false

loader.empty()
loader.addPagesArray([
Expand Down
20 changes: 7 additions & 13 deletions packages/gatsby/cache-dir/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ let hasFetched = Object.create(null)
let syncRequires = {}
let asyncRequires = {}
let jsonDataPaths = {}
let pathPrefix = ``
let pathPrefix = `/`
let fetchHistory = []
let fetchingPageResourceMapPromise = null
let fetchedPageResourceMap = false
Expand Down Expand Up @@ -48,7 +48,7 @@ const fetchResource = resourceName => {
if (resourceName in jsonStore) {
resolve(jsonStore[resourceName])
} else {
const url = `${pathPrefix ? pathPrefix : `/`}static/d/${
const url = `${pathPrefix}static/d/${
jsonDataPaths[resourceName]
}.json`
var req = new XMLHttpRequest()
Expand Down Expand Up @@ -156,7 +156,6 @@ const sortResourcesByCount = (a, b) => {
}

let findPage
let pages = []
let pathScriptsCache = {}
let resourcesArray = []
let mountOrder = 1
Expand All @@ -165,19 +164,14 @@ const queue = {
empty: () => {
resourcesCount = Object.create(null)
resourcesArray = []
pages = []
pathPrefix = ``
pathPrefix = `/`
},

addPagesArray: newPages => {
pages = newPages
if (
typeof __PREFIX_PATHS__ !== `undefined` &&
typeof __PATH_PREFIX__ !== `undefined`
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this if seems to make related tests unhappy ( https://travis-ci.org/gatsbyjs/gatsby/jobs/374034245#L1484 ) :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we don't do this check in other places, I just want to make them consistency, what about change the test code here https://github.com/gatsbyjs/gatsby/blob/v2/packages/gatsby/cache-dir/__tests__/loader.js#L5-L12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should reset them to the proper values like '' and false instead of undefined, also I think we can drop the workaround for Node v6, do we still support that node version?
I see we still support v6 in the package.json, I think we can drop the support for that

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can adjust tests, sure. Just keep in mind that we should check tests (some of them are still failing on v2 but we are working to get them all of the passing again - just let's not break tests that are already passing - either adjust code or tests - whichever make sense)

We want to support node 6 (related comment chain - #2641 (comment) ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests are passing now

if (__PREFIX_PATHS__ === true) pathPrefix = `${__PATH_PREFIX__}/`
if (__PREFIX_PATHS__) {
pathPrefix = `${__PATH_PREFIX__}/`
}
findPage = pageFinderFactory(newPages, pathPrefix)
findPage = pageFinderFactory(newPages, pathPrefix.slice(0, -1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use __PATH_PREFIX__ here? The slicing is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we use slice in other places, when calculating then basename, I just want to keep consistency, I agree it's a bit redundant, but I think we can do it in another pathPrefix refactor which I described in the issues's description

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right :-) You did cover this in the description 👍to that

},
addDevRequires: devRequires => {
syncRequires = devRequires
Expand All @@ -191,7 +185,7 @@ const queue = {
dequeue: () => resourcesArray.pop(),
enqueue: rawPath => {
// Check page exists.
const path = stripPrefix(rawPath, pathPrefix)
const path = stripPrefix(rawPath, pathPrefix.slice(0, -1))

let page = findPage(path)

Expand Down