Feature UPDATE js src #5
Conversation
src/index.jsx
Outdated
| // component from the script's observers before unmounting the component. | ||
| if (observers) { | ||
| delete observers[this.scriptLoaderId]; | ||
| // delete (this.constructor.scriptObservers)[url]; |
There was a problem hiding this comment.
I think It must not be commented, but your latest test fails if I uncomment this line
There was a problem hiding this comment.
I agree. Feel free to fix the tests necessary
|
KeepAlive |
src/index.jsx
Outdated
| @@ -36,53 +40,99 @@ export default class Script extends React.Component { | |||
| constructor(props) { | |||
| super(props); | |||
| this.scriptLoaderId = `id${this.constructor.idCount++}`; // eslint-disable-line space-unary-ops, no-plusplus | |||
| this.pkID = `script-${shortid.generate()}`; | |||
| this.unmount = () => { | |||
There was a problem hiding this comment.
Please make this a separate function in the component instead of creating it in constructor
src/index.jsx
Outdated
| delete (this.constructor.erroredScripts)[url]; | ||
| } | ||
| }; | ||
| this.generate = () => { |
There was a problem hiding this comment.
Please make this a separate function in the component instead of creating it in constructor
src/index.jsx
Outdated
|
|
||
| // If this script has been previously errored | ||
| if (errored) { | ||
| delete (this.constructor.erroredScripts)[url]; |
There was a problem hiding this comment.
No need for the parentheses
src/index.jsx
Outdated
|
|
||
| // If this script has been previously loaded | ||
| if (loaded) { | ||
| delete (this.constructor.loadedScripts)[url]; |
There was a problem hiding this comment.
No need for the parentheses
| } | ||
| shouldComponentUpdate(nextProps) { | ||
| let r = false; | ||
| Object.keys(this.props).forEach((k) => { |
There was a problem hiding this comment.
This can be easily rewritten using Array.prototype.find() - it will shorter and even a bit faster as you won't be iterating the whole array when a positive result is found.
There was a problem hiding this comment.
You can even use some which is even better, something like the following should work
shouldComponentUpdate(nextProps) {
return Object.keys(this.props).some(k => this.props[k] !== nextProps[k]
&& !(typeof this.props[k] === 'function' && typeof nextProps[k] === 'function'))
}
package.json
Outdated
| @@ -10,7 +10,7 @@ | |||
| }, | |||
| "scripts": { | |||
| "build": "npm run build:lib", | |||
| "build:lib": "babel src --out-dir lib", | |||
| "build:lib": "babel src --out-dir lib && uglifyjs ./lib/index.js -o ./lib/index.min.js", | |||
There was a problem hiding this comment.
Is this necessary for a module? Webpack takes care of this for production builds
src/index.jsx
Outdated
| // component from the script's observers before unmounting the component. | ||
| if (observers) { | ||
| delete observers[this.scriptLoaderId]; | ||
| // delete (this.constructor.scriptObservers)[url]; |
There was a problem hiding this comment.
I agree. Feel free to fix the tests necessary
|
Also please update readme :-) |
|
Ok. I'll add your indications this week @jakubkottnauer. |
|
I think it will solve the issue #6 |
|
@andresin87 Hi. Please what is the real world application of this? |
EX: You can overwrite your jQuery version only updating the prop src