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: unhandle error when parameter is not set in v3 and v5 #618

Closed
wants to merge 0 commits into from

Conversation

rzkytmgr
Copy link
Contributor

@rzkytmgr rzkytmgr commented Mar 8, 2022

source only checks if namespace type is equal to string. The error will be triggered when the namespace type is not equal to string unless the namespace parameter presented as an array.

Thank you,

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

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

Can you add a unit test here, to test the undefined / null namespace cases?

src/v35.js Outdated Show resolved Hide resolved
@rzkytmgr
Copy link
Contributor Author

rzkytmgr commented Mar 8, 2022

Can you add a unit test here, to test the undefined / null namespace cases?

of course with pleasure, let me work for it.

@broofa broofa requested a review from ctavan March 8, 2022 20:53
@broofa
Copy link
Member

broofa commented Mar 8, 2022

@ctavan, You okay merging this in this form (with CI tweaked to run on 16.13)? See #619 for details. tl;dr: It's an issue in our example code, not something @rzkytmgr introduced.

@broofa
Copy link
Member

broofa commented Mar 16, 2022

@ctavan ping

@ctavan
Copy link
Member

ctavan commented Mar 16, 2022

Proposed an alternative fix to the CI issue in #621.

I'd prefer to merge that one first to produce a cleaner diff in this PR.

@ctavan
Copy link
Member

ctavan commented Mar 17, 2022

I think I somehow messed this up while rebasing… I created #622 with the same patch instead.

@ctavan ctavan closed this Mar 17, 2022
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.

3 participants