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

Refactor to avoid function constructor in the form of eval #80

Closed
wants to merge 1 commit into from

Conversation

archmoj
Copy link

@archmoj archmoj commented Dec 25, 2020

Seems like this refactor works to avoid security warnings/issues.
Also it would be nice to have a similar patch for d3 v3.
see: archmoj/d3@93e74e5

cc: @curran @tdelmas #67 #65
cc: @alexcjohnson @nicolaskruchten plotly/plotly.js#897

@mbostock
Copy link
Member

Please read #26 (comment).

@mbostock mbostock closed this Dec 25, 2020
@archmoj
Copy link
Author

archmoj commented Dec 26, 2020

Thanks for the note.
The attempt in #26 wasn't as fast while using reduce.
I don't really see a notable performance difference between parsing a 1024x1024 table using current (master) and (this) new implementation.
So please consider reopening.
Cheers.

@tdelmas
Copy link

tdelmas commented Dec 26, 2020

@archmoj Could you provide your results with the latest Chrome and Firefox in the same format that #26 (comment) ?

@mbostock
Copy link
Member

If the substantial performance penalty of doing this “safely” (scare quotes because it’s already safe, just not in a way that a browser can statically analyze and know ahead of time) I’m happy to reconsider. I’m also curious if Object.entries, possibly with a generator rather than a materialized array, is faster than assigning to an object. I doubt it, but sometimes browsers have clever optimizations.

@tyn1998
Copy link

tyn1998 commented Feb 13, 2022

In my case, I'm developing a chrome extension under manifest v3, which refuses CSP with 'unsafe-eval'. So this PR helps! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants