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

Relink less stuff #2375

Merged
merged 5 commits into from
Feb 16, 2018
Merged

Relink less stuff #2375

merged 5 commits into from
Feb 16, 2018

Conversation

alexcjohnson
Copy link
Collaborator

@etpinard a quick change to improve update performance with complicated customdata

@@ -12,6 +12,7 @@ module.exports = function makeSchema(plotlyPath, schemaPath) {
// package is annoying and platform-dependent.
// see https://github.com/tmpvar/jsdom/issues/1782
w.HTMLCanvasElement.prototype.getContext = function() { return null; };
w.URL.createObjectURL = function() { return null; };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@etpinard npm run build works again with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done!

// filter out data_array items that can contain user objects
// most of the time the toVal === fromVal check will catch these early
// but if the user makes new ones we also don't want to recurse in.
if(k === 'customdata' || k === 'ids') continue;
Copy link
Contributor

@etpinard etpinard Feb 16, 2018

Choose a reason for hiding this comment

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

For reference, we should probably right down somewhere that values in the ids arrays should be strings.

But yeah, 👍 for being safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, we should probably right down somewhere that values in the ids arrays should be strings.

I can put that in its attribute description, but is it important that these are strings and not numbers? Do they get converted to actual strings during use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do they get converted to actual strings during use?

yep: cd[i].id = String(trace.ids[i]);

Copy link
Contributor

Choose a reason for hiding this comment

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

I can put that in its attribute description

That would be great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment added in 62e54b7

var accesses = 0;

var obj = {
get _x() { accesses++; return 1; },
Copy link
Contributor

Choose a reason for hiding this comment

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

Really, this is valid ES5?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I believe so, though don't use it in src as it's not supported by IE or Safari. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh actually I was reading the browser compatibility table wrong... in fact looks like this basic setter/getter usage is fine even in IE and Safari, though I still feel like we should avoid it as it can make things confusing. I've only ever used it for testing and debugging. If, for example, a property gets set but you can't figure out where it's getting set, attach a setter for that property that throws an error.

@etpinard
Copy link
Contributor

Looking great 💃

@alexcjohnson alexcjohnson merged commit 552e471 into master Feb 16, 2018
@alexcjohnson alexcjohnson deleted the relink-less-stuff branch February 16, 2018 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants