-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: node support #75
Conversation
@@ -60,6 +62,7 @@ | |||
"doctoc": "^1.3.1", | |||
"husky": "^1.0.0-rc.13", | |||
"jsdoc": "https://github.com/BrandonOCasey/jsdoc#feat/plugin-from-cli", | |||
"jsdom": "^15.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock document in node
"test": "karma start scripts/karma.conf.js", | ||
"test": "npm-run-all test:browser test:node", | ||
"test:browser": "karma start scripts/karma.conf.js", | ||
"test:node": "qunit test/dist/bundle-node.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run tests in node
package.json
Outdated
"global": "^4.3.2", | ||
"url-toolkit": "^2.1.1" | ||
"url-toolkit": "^2.1.1", | ||
"xmldom": "^0.1.27" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node does not have access to window.DOMParser
return defaults; | ||
}, | ||
externals(defaults) { | ||
defaults.module.push('xmldom'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
externals for the module
output are modules that should be required
rather than built into the bundle.
return defaults; | ||
}, | ||
globals(defaults) { | ||
defaults.browser.xmldom = 'window'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window
provides DOMParser
for the browser
defaults.browser.atob = 'window.atob'; | ||
defaults.test.xmldom = 'window'; | ||
defaults.test.atob = 'window.atob'; | ||
defaults.test.jsdom = '{JSDOM: function() { return {window: window}; }}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsdom is a bit weird, see the usage in tests for why we mock it like this.
src/index.js
Outdated
parseUTCTimingScheme(stringToMpdXml(manifestString)); | ||
parseUTCTimingScheme(_stringToMpdXml(manifestString)); | ||
|
||
export const stringToMpdXml = _stringToMpdXml; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export all to-level functions just in case they don't want to use toM3u8
@@ -1,12 +1,12 @@ | |||
import window from 'global/window'; | |||
import {DOMParser} from 'xmldom'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this becomes import {DOMParser} from 'window';
for the browser. via rollup globals
import QUnit from 'qunit'; | ||
import { parseAttributes } from '../src/parseAttributes'; | ||
|
||
const document = new JSDOM().window.document; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSDOM's strange usage.
deef961
to
83529ca
Compare
83529ca
to
0d32573
Compare
src/index.js
Outdated
import { toPlaylists } from './toPlaylists'; | ||
import { inheritAttributes } from './inheritAttributes'; | ||
import { stringToMpdXml } from './stringToMpdXml'; | ||
import { toM3u8 as _toM3u8} from './toM3u8'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we can export them with there "real name" lower in the file. export const toM3u8 = _toM3u8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, you should be able to do the following and avoid the prefixes:
export {
toM3u8,
inheritAttributes,
...
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will test with hls-fetcher to see if it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the change
Brought up due to videojs/hls-fetcher#35
Depends on: videojs/vhs-utils#1Out as 1.1.0, and now in this pull request.