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

Decorate requests with symbols with apply=true #3996

Merged
merged 1 commit into from
Dec 28, 2019

Conversation

alexghr
Copy link
Contributor

@alexghr alexghr commented Nov 12, 2019

Hello,

This PR fixes a bug where Symbol decorations would not get added to request instances when { apply: true } option was used. The issue was caused by iterating over decorations object using for...in which only iterates over enumerable properties (symbols are not enumerable)

Since Symbols are expected property names this PR fixes the issue above by also iterating over known symbol properties using Object.getOwnPropertySymbols.

@kanongil
Copy link
Contributor

kanongil commented Nov 13, 2019

I would probably refactor requestApply, along with the other _decorations entries, to use a Map(). This makes it explicit which properties are decorated, and should fix this bug.

Use map to hold decorations internally. This also fixes a bug where
request decorations using Symbols did not work with `apply=true`
@alexghr
Copy link
Contributor Author

alexghr commented Nov 18, 2019

Thanks for the suggestion @kanongil. It simplifies the code greatly! I'm not very familiar with the codebase but I've tried my best at refactoring the internal _decorations object to use a series of maps.

The test still pass apart from two of them, although they were failing even before this change.

@hueniverse hueniverse self-assigned this Dec 28, 2019
@hueniverse hueniverse added the bug Bug or defect label Dec 28, 2019
@hueniverse hueniverse added this to the 19.0.0 milestone Dec 28, 2019
@hueniverse
Copy link
Contributor

It's rare I merge a PR... nicely done.

@hueniverse hueniverse merged commit 390b318 into hapijs:master Dec 28, 2019
@hueniverse hueniverse mentioned this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants