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

Fix bug where docpress would not load external scripts #172

Merged
merged 2 commits into from
Nov 1, 2016

Conversation

victorquinn
Copy link
Contributor

Due to the fact that docpress expected all scripts to be in the files array (consisting of local files only), it was not properly adding script tags for external scripts.

In fact providing an external url would cause docpress to crash with the following error since an external url would not be in the files object:

        if (!file.contents) return
                 ^

TypeError: Cannot read property 'contents' of undefined
    at scripts.map (/Users/victor/Documents/Development/chancejs/node_modules/docpress-base/index.js:134:18)
    at Array.map (native)
    at callback (/Users/victor/Documents/Development/chancejs/node_modules/docpress-base/index.js:128:33)
    at /Users/victor/Documents/Development/chancejs/node_modules/browserify/index.js:778:13
    at ConcatStream.<anonymous> (/Users/victor/Documents/Development/chancejs/node_modules/concat-stream/index.js:36:43)
    at emitNone (events.js:91:20)
    at ConcatStream.emit (events.js:185:7)
    at finishMaybe (/Users/victor/Documents/Development/chancejs/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:475:14)
    at endWritable (/Users/victor/Documents/Development/chancejs/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:485:3)
    at ConcatStream.Writable.end (/Users/victor/Documents/Development/chancejs/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:455:41)

This fixes the bug to ensure that Docpress will add a script tag for a scripts at an external url and that it no longer crashes when provided such a url.

let fileHash = hash(file.contents)
return `${location}?t=${fileHash}`
// Need to check if local or external
let fileUrl = new url.parse(location, false, true)
Copy link
Member

Choose a reason for hiding this comment

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

no new required here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops good catch!

I first tried another module then moved to the core node one to prevent an additional dependency and accidentally left this.

Will fix as soon as I'm able!

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage decreased (-1.2%) to 81.152% when pulling adf5f66 on victorquinn:master into dd0e548 on docpress:master.

@knownasilya knownasilya merged commit 8aceef1 into docpress:master Nov 1, 2016
@knownasilya
Copy link
Member

I've released the latest changes as v0.7.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants