-
Notifications
You must be signed in to change notification settings - Fork 88
Fix output for es5 #408
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
Fix output for es5 #408
Conversation
@Spy-Seth this seems good for me, tests are passing 🤷♂ thoughts?
I'm not fan to use a community fork of stringify-object
only dedicated to this package. If we use a fork we should manage it (as there is no strong alternative package). @vvo Could we fork it and publish it from the Algolia org?
To clarify my answer from yesterday, I'm all in favor of resolving the stringify-object
bundling issue. It's a pain in the ass for us for too long: source of bugs in the browser and maintenance headaches (we are stuck in the rollup upgrade because of it...)
Hi @Spy-Seth, @vvo , let me know if you want me to make changes to this once you have decided whether to publish an internal fork.
@vvo Could we fork it and publish it from the Algolia org?
I would rather use the one from @Chris-Baker since it seems he had the issue and he would be the best to maintain it. There's also https://github.com/searls/stringify-object/tree/transform-es5 which already existed too if we want to reuse something and if @Chris-Baker want to work with @searls.
Latest release from stringify-object was one year ago, I believe it's fine to just merge this if the only issue is about who owns the fork. Or reuse https://github.com/searls/stringify-object/tree/transform-es5.
@Spy-Seth I won't be at Algolia forever and don't want Algolia to own something they did not need (stringify object es5) but rather trust the community (@Chris-Baker) to do good.
404fda7
to
e3a3284
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is 👌 for me.
I just remove some stringify-object
rollup configuration and rebase the PR on master.
Let's merge this. Thanks a lot for the work @Chris-Baker
Fix for #387 to replace stringify-object and make the output compatible with ES5.