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

Reduce json stringify #920

Closed
wants to merge 1 commit into from
Closed

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 15, 2023

We can reduce the amount of JSON.stringify calls for the obvious cases. ;)

@G-Rath
Copy link
Member

G-Rath commented Nov 15, 2023

I don't think this is a good change - we're using JSON.stringify exactly what it's meant for: so we don't have to manually craft JSON or worry about escaping etc; while yes these cases might be safer to write out directly, it means going forward we should be making people decide "is this JSON simple enough to not use JSON.stringify?".

It's also possible refractors could result in dynamic content that needs to be escaped gets introduced, which JSON.stringify would handle for us - better to just always use JSON.stringify and not have to think about it

@gr2m
Copy link
Contributor

gr2m commented Nov 15, 2023

I agree with @G-Rath. I don't think the optimization is worth it for code readability / maintainability reasons. JSON.stringify is very efficient in modern runtimes.

@Uzlopak Uzlopak closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants