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: return entries instead of values in toJSON method #9345

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

Shiva953
Copy link
Contributor

@Shiva953 Shiva953 commented Apr 8, 2023

Please describe the changes this PR makes and why it should be merged:
This PR modifies the toJSON method in the collection package to return entries instead of just values. Currently, the toJSON method only returns values from the collection, which makes it impossible to create a new collection from the JSON output.

To fix this issue, this PR modifies the toJSON method to return entries instead of just values. This will preserve the keys and allow the creation of a new collection/map from the returned data. (FIXES #9310 ).

Status and versioning classification:
- Code changes have been tested against the Discord API, or there are no code changes
- This PR changes the library's interface (methods or parameters added)
- (Status Classification) This PR includes breaking changes (method(s) removed or renamed, parameters moved or removed)

Please review this PR and let me know if any changes are required.

@vercel
Copy link

vercel bot commented Apr 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2023 1:27am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 1:27am

@vercel
Copy link

vercel bot commented Apr 8, 2023

@Shiva953 is attempting to deploy a commit to the discordjs Team on Vercel.

A member of the Team first needs to authorize it.

@jaw0r3k
Copy link
Contributor

jaw0r3k commented Apr 8, 2023

Well it's a braking change, so you should mention about it

@Shiva953
Copy link
Contributor Author

Shiva953 commented Apr 8, 2023

Well it's a braking change, so you should mention about it

Can you please Elaborate on what I should mention?

@jaw0r3k
Copy link
Contributor

jaw0r3k commented Apr 8, 2023

Can you please Elaborate on what I should mention?

The status classification
- This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@Shiva953
Copy link
Contributor Author

Shiva953 commented Apr 8, 2023

Can you please Elaborate on what I should mention?

The status classification
- This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

Done👍

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but yes, very breaking

Copy link
Member

@iCrawl iCrawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests needs updating

@kyranet
Copy link
Member

kyranet commented Oct 9, 2023

@Shiva953 your branch update missed updating toJSON()'s tests, CI is reporting errors:

image

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 78
🟢 Accessibility 98
🟠 Best practices 83
🟠 SEO 86
🟠 PWA 67

Lighthouse ran on https://discord-js-guide-git-fork-shiva953-feature-discordjs.vercel.app/guide/home/introduction

@Jiralite Jiralite changed the title feat(collection): return entries instead of values in toJSON method feat: return entries instead of values in toJSON method Oct 31, 2023
@Jiralite Jiralite removed the blocked label Nov 7, 2023
@Jiralite Jiralite added this to the collection 2.0 milestone Nov 7, 2023
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #9345 (1169412) into main (6b9f906) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #9345   +/-   ##
=======================================
  Coverage   60.14%   60.14%           
=======================================
  Files         234      234           
  Lines       16241    16241           
  Branches     1234     1234           
=======================================
  Hits         9768     9768           
  Misses       6429     6429           
  Partials       44       44           
Flag Coverage Δ
collection 99.28% <100.00%> (ø)
next ∅ <ø> (∅)
proxy 75.00% <ø> (ø)
rest 92.77% <ø> (ø)
ws 52.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/collection/src/collection.ts 99.28% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@kodiakhq kodiakhq bot merged commit defeee5 into discordjs:main Nov 8, 2023
6 checks passed
cobaltt7 added a commit to scratchaddons-community/scradd that referenced this pull request Dec 19, 2023
Signed-off-by: RedGuy12 <paul@reid-family.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Change collection#toJSON to return entries instead of values only
8 participants