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.

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

Check for .json file extension before JSON parse. Was throwing when r… #601

Closed
wants to merge 2 commits into from

Conversation

msand
Copy link

@msand msand commented Jan 1, 2017

…eading js files. Related to #597


cache[f] = component
return component
if (f.slice(-5) === '.json') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is always .json ? @arunoda

Copy link
Contributor

Choose a reason for hiding this comment

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

But actually, SyntaxError: Unexpected token m in JSON at position 0 this error looks like trying to parse a JS file (module.exports =....).

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to have .js as well. In order to support dist. Loading SSR files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! wait. But for here, it's always .json.
If there's no .json file, we need to throw an error.
Then I think we don't need a change here.

@ryancole
Copy link

ryancole commented Jan 1, 2017

Just tested msand:fix-windows-error, and it does indeed cause the error to go away. I'm on Windows 10.

@@ -12,11 +12,15 @@ async function readPage (path) {
return cache[f]
}

const source = await fs.readFile(f, 'utf8')
const { component } = JSON.parse(source)
let content = await fs.readFile(f, 'utf8')
Copy link
Contributor

Choose a reason for hiding this comment

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

@msand Would you mind removing all the changes done to this page.
We always need to read .json files so if there's no .json file, that's an error.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I figured as much, should've made a separate pr to begin with. Thanks

@arunoda
Copy link
Contributor

arunoda commented Jan 2, 2017

Took this PR with #611

@arunoda arunoda closed this Jan 2, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants