Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

node:zlib implem: Expose zlib.constants #2496

Merged
merged 1 commit into from
Aug 12, 2024
Merged

node:zlib implem: Expose zlib.constants #2496

merged 1 commit into from
Aug 12, 2024

Conversation

npaun
Copy link
Member

@npaun npaun commented Aug 7, 2024

No description provided.

@npaun npaun requested review from jasnell and anonrig August 7, 2024 20:27
@npaun npaun requested review from a team as code owners August 7, 2024 20:27
Copy link

github-actions bot commented Aug 7, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@npaun
Copy link
Member Author

npaun commented Aug 7, 2024

I have read the CLA Document and I hereby sign the CLA

@npaun
Copy link
Member Author

npaun commented Aug 7, 2024

recheck

github-actions bot added a commit that referenced this pull request Aug 7, 2024
src/node/internal/zlib.d.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/workerd/api/node/tests/zlib-nodejs-test.js Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
@npaun npaun force-pushed the npaun/zlib-sync-1 branch from 89ad8c7 to 637d66b Compare August 7, 2024 22:02
src/node/internal/zlib.d.ts Outdated Show resolved Hide resolved
@npaun npaun force-pushed the npaun/zlib-sync-1 branch from 637d66b to e5ba13c Compare August 8, 2024 18:09
"BROTLI_DECODER_ERROR_ALLOC_RING_BUFFER_1": -26,
"BROTLI_DECODER_ERROR_ALLOC_RING_BUFFER_2": -27,
"BROTLI_DECODER_ERROR_ALLOC_BLOCK_TYPE_TREES": -30,
"BROTLI_DECODER_ERROR_UNREACHABLE": -31
Copy link
Member

Choose a reason for hiding this comment

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

nit: if constants are added later it would be easy for this list to get out of date. I wonder if it would make sense to have the test fail if there are any unexpected constants in the constants object. You can check for that fairly easily by grabbing the Object.keys() and removing each item that is explicitly tested here then throwing if there are any left over at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting to test for this list of keys, and not check the values, or am I misunderstanding the suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

For for the list of keys and the values, but throw if there are additional keys in constants not tested for explicitly.

src/workerd/api/BUILD.bazel Outdated Show resolved Hide resolved
@npaun npaun force-pushed the npaun/zlib-sync-1 branch from e5ba13c to 6eb4389 Compare August 8, 2024 20:51
@npaun
Copy link
Member Author

npaun commented Aug 8, 2024

Thanks for the suggestions, guys. It should be ready to go now.

@npaun npaun requested review from jasnell, anonrig and fhanau August 8, 2024 20:51
@npaun npaun force-pushed the npaun/zlib-sync-1 branch from 6eb4389 to f073bac Compare August 8, 2024 21:05
@npaun npaun mentioned this pull request Aug 8, 2024
21 tasks
@npaun npaun force-pushed the npaun/zlib-sync-1 branch 2 times, most recently from 5364360 to b171997 Compare August 8, 2024 21:58
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
@npaun npaun force-pushed the npaun/zlib-sync-1 branch 2 times, most recently from 46658ff to 2f40c42 Compare August 12, 2024 14:54
@jasnell
Copy link
Member

jasnell commented Aug 12, 2024

If you would, rebase this PR on main again and let's give the internal PR CI run another go.

@npaun npaun force-pushed the npaun/zlib-sync-1 branch from 2f40c42 to 3420df7 Compare August 12, 2024 16:31
@npaun
Copy link
Member Author

npaun commented Aug 12, 2024

@jasnell Done. However I still didn't sort out the zlib version inconsistency so I think it'll fail again.

@jasnell
Copy link
Member

jasnell commented Aug 12, 2024

That's fine, I just noticed that the internal streams.ew-test failed and it's likely because of the commit that got reverted this morning. The rebase at least helps to rule that out

@npaun npaun force-pushed the npaun/zlib-sync-1 branch 2 times, most recently from daf6c11 to 3c66f2b Compare August 12, 2024 20:56
@npaun npaun force-pushed the npaun/zlib-sync-1 branch from 3c66f2b to ad2cb1c Compare August 12, 2024 20:58
@npaun npaun merged commit 938fd9d into main Aug 12, 2024
9 of 10 checks passed
@fhanau fhanau deleted the npaun/zlib-sync-1 branch August 12, 2024 23:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants