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

feat: updating to use yaml #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sakulstra
Copy link

@sakulstra sakulstra commented Jun 23, 2022

use yaml instead of js-yaml
as requested in #145

@jonschlinkert are you sure yaml is smaller than js-yaml (for some reason bundlephobia is broken for yaml and vscode showing bigger package size, might be wrong due to shaking though)

yaml seems to behave slightly different as you can see in the two failing tests.

@sakulstra sakulstra marked this pull request as ready for review July 10, 2022 19:56
@sakulstra
Copy link
Author

sakulstra commented Jul 10, 2022

@jonschlinkert with #148 applied tests are passing.

@jonschlinkert
Copy link
Owner

jonschlinkert commented Jul 11, 2022

are you sure yaml is smaller than js-yaml (for some reason bundlephobia is broken for yaml and vscode showing bigger package size

I'm not at all sure about that lol. Thank you for doing this and for bringing it to my attention. I was initially under the impression that yaml would be smaller, since js-yaml was massively bloated the last time I checked. They included all of their tests, test fixtures, browser code, etc. in the published package. Now that you have seen both, what is your recommendation?

edit: and also, thank you for taking the time to do this, and apologies for making you jump through hoops. I promise we'll get this merged and published as soon as we settle on which library to use.

@sakulstra
Copy link
Author

Would in fact go with yaml - seems more solid to me.

@sakulstra
Copy link
Author

@jonschlinkert any chance of getting this merged? 😅

@dralshehri dralshehri mentioned this pull request Aug 6, 2022
@sakulstra
Copy link
Author

@jonschlinkert ping 😅

@sakulstra
Copy link
Author

@jonschlinkert 😿 - quite a lot of ppl are using this library (1M downloads per week). Would be really good if this could be merged anytime soon.

@kingjollie kingjollie mentioned this pull request Sep 27, 2022
@sakulstra sakulstra changed the title feat: try updating to use yaml feat: updating to use yaml Nov 15, 2022
@sakulstra
Copy link
Author

@jonschlinkert ping on this, let me know if anything prevents merging.

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

Successfully merging this pull request may close these issues.

None yet

2 participants