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

Feature UPDATE js src #5

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary for a module? Webpack takes care of this for production builds

Copy link
Author

Choose a reason for hiding this comment

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

ok

"clean": "rimraf lib",
"lint": "eslint ./src/*",
"test": "jest",
Expand Down Expand Up @@ -43,10 +43,13 @@
"prop-types": "15.5.8",
"react": "15.5.4",
"react-addons-test-utils": "15.5.1",
"react-dom": "15.5.4",
"rimraf": "2.4.3"
"react-dom": "^15.5.4",
"react-test-renderer": "^15.6.1",
"rimraf": "2.4.3",
"uglify-js": "^3.0.17"
},
"dependencies": {
"shortid": "^2.2.8"
},
"peerDependencies": {
"react": ">=0.14.9",
Expand Down
114 changes: 82 additions & 32 deletions src/index.jsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
import React from 'react';
import { PropTypes as RPT } from 'prop-types';

import * as shortid from 'shortid';

export default class Script extends React.Component {

static propTypes = {
onCreate: RPT.func,
onError: RPT.func.isRequired,
onLoad: RPT.func.isRequired,
onUnload: RPT.func.isRequired,
url: RPT.string.isRequired,
};

static defaultProps = {
onCreate: () => {},
onError: () => {},
onLoad: () => {},
}
onCreate: () => null,
onError: () => null,
onLoad: () => null,
onUnload: () => null,
};

// A dictionary mapping script URLs to a dictionary mapping
// component key to component for all components that are waiting
Expand All @@ -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 = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a separate function in the component instead of creating it in constructor

const { url, onUnload } = this.props;
const observers = this.constructor.scriptObservers[url];
const loaded = this.constructor.loadedScripts[url];
const errored = this.constructor.erroredScripts[url];

// If the component is waiting for the script to load, remove the
// component from the script's observers before unmounting the component.
if (observers) {
delete observers[this.scriptLoaderId];
// delete (this.constructor.scriptObservers)[url];
Copy link
Author

Choose a reason for hiding this comment

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

I think It must not be commented, but your latest test fails if I uncomment this line

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Feel free to fix the tests necessary

}

// If this script has been previously loaded
if (loaded) {
delete (this.constructor.loadedScripts)[url];
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the parentheses

const s = document.querySelector(`[data-pkID="${this.pkID}"]`);
s.parentNode.removeChild(s);
onUnload();
}

// If this script has been previously errored
if (errored) {
delete (this.constructor.erroredScripts)[url];
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the parentheses

}
};
this.generate = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a separate function in the component instead of creating it in constructor

const { onError, onLoad, url } = this.props;
if (this.constructor.loadedScripts[url]) {
onLoad();
return;
}

if (this.constructor.erroredScripts[url]) {
onError();
return;
}

// If the script is loading, add the component to the script's observers
// and return. Otherwise, initialize the script's observers with the component
// and start loading the script.
if (this.constructor.scriptObservers[url]) {
this.constructor.scriptObservers[url][this.scriptLoaderId] = this.props;
return;
}

this.constructor.scriptObservers[url] = { [this.scriptLoaderId]: this.props };

this.createScript();
};
}

componentDidMount() {
const { onError, onLoad, url } = this.props;

if (this.constructor.loadedScripts[url]) {
onLoad();
return;
}

if (this.constructor.erroredScripts[url]) {
onError();
return;
}
this.generate();
}

// If the script is loading, add the component to the script's observers
// and return. Otherwise, initialize the script's observers with the component
// and start loading the script.
if (this.constructor.scriptObservers[url]) {
this.constructor.scriptObservers[url][this.scriptLoaderId] = this.props;
return;
}
shouldComponentUpdate(nextProps) {
let r = false;
Object.keys(this.props).forEach((k) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

@jakubkottnauer jakubkottnauer Jun 30, 2017

Choose a reason for hiding this comment

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

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'))
}

if (r) return;
if (
this.props[k] !== nextProps[k] &&
!(typeof this.props[k] === 'function' && typeof nextProps[k] === 'function')
) {
r = true;
}
});
return r;
}

this.constructor.scriptObservers[url] = { [this.scriptLoaderId]: this.props };
componentWillUpdate() {
this.unmount();
}

this.createScript();
componentDidUpdate() {
this.generate();
}

componentWillUnmount() {
const { url } = this.props;
const observers = this.constructor.scriptObservers[url];

// If the component is waiting for the script to load, remove the
// component from the script's observers before unmounting the component.
if (observers) {
delete observers[this.scriptLoaderId];
}
this.unmount();
}

createScript() {
const { onCreate, url } = this.props;
const { pkID } = this;
const script = document.createElement('script');

onCreate();

script.src = url;
script.async = 1;
script.setAttribute('data-pkID', pkID);

const callObserverFuncAndRemoveObserver = (shouldRemoveObserver) => {
const observers = this.constructor.scriptObservers[url];
Expand Down