Skip to content

n-api: code is optional on create error#399

Merged
yorkie merged 1 commit intomasterfrom
n-api/create-error
Oct 31, 2018
Merged

n-api: code is optional on create error#399
yorkie merged 1 commit intomasterfrom
n-api/create-error

Conversation

@legendecas
Copy link
Copy Markdown
Contributor

  • npm test passes
  • tests and/or benchmarks are included

According to N-API docs, second argument of napi_create_error/napi_create_type_error/napi_create_range_error shall be an optional string typed JS value.

Copy link
Copy Markdown
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

Leave comments :)

/** code has to be an JS string type, thus it can not be an number 0 */ \
if (code != NULL) { \
NAPI_TRY_TYPE(string, jval_code); \
iotjs_jval_set_property_jval(jval_error, "code", jval_code); \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add code as MAGIC string?

\
iotjs_jval_set_property_jval(jval_error, "code", jval_code); \
/** code has to be an JS string type, thus it can not be an number 0 */ \
if (code != NULL) { \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Validate it in JerryScript API?

Copy link
Copy Markdown
Contributor Author

@legendecas legendecas Oct 31, 2018

Choose a reason for hiding this comment

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

it has been validated on next line only when code is not NULL - that is - code is given a value.

Copy link
Copy Markdown
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

Looks good to me

@yorkie yorkie merged commit 0b3aae7 into master Oct 31, 2018
@yorkie yorkie deleted the n-api/create-error branch October 31, 2018 07:09
yorkie pushed a commit that referenced this pull request Oct 31, 2018
legendecas pushed a commit that referenced this pull request Oct 31, 2018
* build: use CMAKE_INSTALL_PREFIX instead of custom install command (#391)

* build: add dependencies for iotjs.cmake which depends on others (#396)

Within some parallel builds, it occasionally gets a build error on
libmqtt_packet depenedency.

* events: cannot read property 'newListener' of undefined (#397)

* tools: clang-format-7 on travis-ci (#398)

* mqtt: update event trigger timing (#400)

* fix/mqtt-connect-event

* remove abundant error listener

* n-api: code is optional on create error (#399)

* v0.10.13: bump version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants