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

fix: add check that $ref value is a string (#92) #94

Conversation

aeworxet
Copy link
Collaborator

@aeworxet aeworxet commented Nov 2, 2022

This PR adds to function isExternalReference() additional check if $ref's value passed to it has the type of string (adds type guard for JS version, to which TS code will be transpiled when NPM package is released).

Except general usefulness, it is also a possible indirect fix of an issue reported by user in
https://asyncapi.slack.com/archives/CQVJXFNQL/p1666700935030749
(excerpt from the conversation is mirrored in repository's issue #92).

Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
magicmatatjahu
magicmatatjahu previously approved these changes Nov 2, 2022
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

You should add some simple test with AsyncAPI document with defined some messages with $ref and without it in .channels.X.publish/subcribe.message and only check if main function throws or not error, ok? :)

Souvikns
Souvikns previously approved these changes Nov 2, 2022
@Souvikns
Copy link
Member

Souvikns commented Nov 2, 2022

You should add some simple test with AsyncAPI document with defined some messages with $ref and without it in .channels.X.publish/subcribe.message and only check if main function throws or not error, ok? :)

I checked this, and bundler just ignores it and resolves it normally, that is fetches the references and replaces them with the original values. Only $refs under $.channels.*.*.message will be fetched and resolved under #components/messages

aeworxet and others added 4 commits November 3, 2022 08:09
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
@aeworxet aeworxet dismissed stale reviews from Souvikns and magicmatatjahu via 8fab690 November 7, 2022 06:33
magicmatatjahu
magicmatatjahu previously approved these changes Nov 7, 2022
@magicmatatjahu
Copy link
Member

@Souvikns or @derberg Could you make review? I cannot accept 😅

Souvikns
Souvikns previously approved these changes Nov 7, 2022
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@aeworxet can you update description and point to related Slack discussion?

also, if test description refers to error throwing, shouldn't tests do also not.toThrowError() ?

@aeworxet aeworxet dismissed stale reviews from Souvikns and magicmatatjahu via e42cf8d November 7, 2022 15:37
@aeworxet
Copy link
Collaborator Author

aeworxet commented Nov 7, 2022

Changed tests to logic, that if async function resolved Promise, then it did not throw. All other variants were not acceptable for different reasons. One was even crashing Jest itself.

@aeworxet
Copy link
Collaborator Author

aeworxet commented Nov 8, 2022

@derberg Updated PR's description to point to related Slack discussion.

@aeworxet
Copy link
Collaborator Author

aeworxet commented Nov 9, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 5491120 into asyncapi:master Nov 9, 2022
@aeworxet aeworxet deleted the fix-add-check-that-ref-value-is-a-string-#92 branch November 9, 2022 12:02
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.3.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants