Hoc pattern, test fixes and clean-ups #34
Conversation
| ); | ||
| assert.equal(hasScript(URL), true); | ||
|
|
||
| assert.equal(hasScript(URL), false, "Url not in document"); |
There was a problem hiding this comment.
this one was "wrong" before... it was checking the previous tests url instead of verifying that this tests url was not in the document
|
|
||
| const documentErrorScript = (URL) => { | ||
| const script = getScript(URL); | ||
| script.onerror(); |
There was a problem hiding this comment.
added some util functions for the new tests
| const URL = "http://example.com"; | ||
| const ComponentWrapper = makeAsyncScriptLoader(MockedComponent, URL); | ||
| it("should return a component that contains the passed component and fire asyncScriptOnLoad", () => { | ||
| const URL = "http://example.com/?default=true"; |
There was a problem hiding this comment.
made each URL unique for each test. this helps prevent mix ups like the one cleaned up below 😺
| const instance = ReactTestUtils.renderIntoDocument( | ||
| <ComponentWrapper /> | ||
| ); | ||
| assert.equal(hasScript(URL), true); | ||
| ReactDOM.unmountComponentAtNode(ReactDOM.findDOMNode(instance)); | ||
| instance.componentWillUnmount(); |
There was a problem hiding this comment.
added .parentNode to the unmountComponentAtNode call which now correctly removes the mounted component so the components componentWillUnmount is called correctly 😺
| import PropTypes from "prop-types"; | ||
|
|
||
| let SCRIPT_MAP = {}; | ||
|
|
||
| // A counter used to generate a unique id for each component that uses the function | ||
| let idCount = 0; | ||
|
|
||
| export default function makeAsyncScript(Component, getScriptURL, options) { | ||
| export default function makeAsyncScript(getScriptURL, options) { |
There was a problem hiding this comment.
The below diff looks intense, but mostly just indentation due to HOC 😢
biggest changes are:
- the added
(props, context)to theconstructor - added comments to steps inside of
componentDidMount - passing the
entryobject toasyncScriptOnLoad
There was a problem hiding this comment.
can remove the whitespace from the diff => https://github.com/dozoisch/react-async-script/pull/34/files?w=1 . w=1 makes it easier to review in this case :)
There was a problem hiding this comment.
^ that's awesome... i need to remember that for all time.. lol
|
Yeah I think we can remove the old IE check. Yeah it should've been an anonymous function instead of arrow lol.. guess it got linted at some point and I didn't notice. |
|
LGTM! I'll let you do the merge in case there is something else you want to do before merging :) |
|
@dozoisch yeah.. need to update the documentation 😸 thank you for the awesome and timely review! |
| @@ -69,8 +82,7 @@ You can still retrieve the child component using `getComponent()`. | |||
| ### Example | |||
|
|
|||
| ```js | |||
| const MockedComponent = React.createClass({ | |||
There was a problem hiding this comment.
oh wow that was an old example X) Didn't realize I never update it to classes.
"Step 1" of #33
asyncScriptOnLoadonloadandonerrorentryobject toasyncScriptOnLoadobserverskey? just haveloaded/erroredand maybe add theurl?TODO:
@dozoisch Let me know about the question above and if you have any other questions or updates.
also removed the old ie support check. it was actually broken anyway 😭
onreadystatechangehas the wrong reference forthis:should be:
reference: https://www.html5rocks.com/en/tutorials/speed/script-loading/
With
v1.0.0release time to say goodbye to old ie<10?