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

Using querystringparser #10

Closed
tjconcept opened this issue Dec 22, 2014 · 15 comments
Closed

Using querystringparser #10

tjconcept opened this issue Dec 22, 2014 · 15 comments

Comments

@tjconcept
Copy link

I noted that you are using the querystring module and not you own querystringparser (when I tried to use this with a nested object), and I'm just curious about why it is so? Is it to keep it a drop-in-replacement of url?

@petkaantonov
Copy link
Owner

It's because this is supposed to be rewrite of the url module and be integrated into node core

@tjconcept
Copy link
Author

Ah, okay, and "they" are not keen on supporting nesting?
Would make sense to increase the performance of the query string parser/formatter as well, wouldn't it?

@petkaantonov
Copy link
Owner

I suppose if they are not taking drop in replacements for 30x better performance, they are not too keen on changing the APIs of stable modules either :P

@tjconcept
Copy link
Author

Sounds plausible.. What a shame - we might see more from io.js.
I guess they could argue that anything that lessens readability is better suited for user land.
Anyway, I don't mind using user land alternatives, but it would be nice to avoid the if (url.query) url.search = '?' + qs.stringify(url.query);-workaround i currently have.
Btw. I am pretty inspired by your work 👍

@petkaantonov
Copy link
Owner

What's that workaround for?

@tjconcept
Copy link
Author

It's for url.format on an object with a query with nested objects.

I use an object of this format to represent state in my web app and on each change I push this state to the history (using url.format). The url.query quite often has some nesting, e.g. in search or filtering.

@petkaantonov
Copy link
Owner

Actually I could just accept injection of a querystring implementation e.g.

url.qsParser = myQsParser

@petkaantonov petkaantonov reopened this Dec 23, 2014
@tjconcept
Copy link
Author

Good enough for me :)

@mikeal
Copy link

mikeal commented Feb 1, 2015

What a shame - we might see more from io.js.

Are there any API breaks? I can't imagine any reason a 30x perf improvement would be rejected provided it doesn't break or change API :)

@petkaantonov
Copy link
Owner

Are there any API breaks? I can't imagine any reason a 30x perf improvement would be rejected provided it doesn't break or change API :)

It depends if you consider the implementation of a property as part of the API (data property or getter)

@mikeal
Copy link

mikeal commented Feb 1, 2015

There's more than one issue swirling around about this so I already made this comment somewhere else but: can't we lazily produce identical output to the current API on stringify/jsonify and iterable? We can eat the perf hit when people touch these edge cases without losing the perf benefits of this approach.

@petkaantonov
Copy link
Owner

Sure you can define a toJSON method that emulates the output that an eagerly parsed url object would give. JSON.stringify implicitly calls it.

@mikeal
Copy link

mikeal commented Feb 1, 2015

Would we still have iterable issues? I haven't gotten deep enough in to property access guts to know if that will also be an issue but I'm pretty sure that we have a way to emulate it now in ES6.

@petkaantonov
Copy link
Owner

The getters are currently not enumerable (that is of course trivial to change) but even after changing them to enumerable, they are still on the prototype and not on the object itself which trips up stuff like ._extend.

@jedwards1211
Copy link

jedwards1211 commented Dec 14, 2018

@petkaantonov

It's because this is supposed to be rewrite of the url module and be integrated into node core

Just curious, did it ever get merged into node core?

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

No branches or pull requests

4 participants