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

feat: Initial implementation of assert module #668

Merged
merged 12 commits into from
Nov 13, 2024

Conversation

nabetti1720
Copy link
Contributor

@nabetti1720 nabetti1720 commented Nov 7, 2024

Issue # (if available)

#628

Description of changes

The following Node.js APIs, which are also used in the libraries on which aws-xray-sdk-core depends, have been implemented. Note that we do not intend to support all APIs from the beginning.

  • assert.ok()

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Sytten
Copy link
Contributor

Sytten commented Nov 7, 2024

If you can typing that is always appreciated

Copy link
Contributor

@richarddavison richarddavison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I think a basic assert module is good to have!

modules/llrt_assert/src/lib.rs Outdated Show resolved Hide resolved
modules/llrt_assert/src/lib.rs Show resolved Hide resolved
@nabetti1720
Copy link
Contributor Author

nabetti1720 commented Nov 11, 2024

The following verification is still the same before and after the merge of #673 results, with no problems.

// index16.js
import assert from 'assert';
console.log(assert);
assert.ok(true);
assert(true);
% llrt index16.js
[function: (anonymous)]

% bun index16.js 
[Function: ok]

Will the following events be corrected this time?

// index16.js
const assert = require('assert');
console.log(assert);
assert.ok(true);
assert(true);
% llrt index16.js
{
  default: [function: (anonymous)],
  ok: [function: (anonymous)]
}
TypeError: not a function
  at <anonymous> (/Users/shinya/Workspaces/llrt-test/index16.js:4:1)

% bun index16.js 
[Function: ok]

Copy link
Contributor

@richarddavison richarddavison left a comment

Choose a reason for hiding this comment

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

Some comments

modules/llrt_assert/src/lib.rs Outdated Show resolved Hide resolved
modules/llrt_assert/src/lib.rs Outdated Show resolved Hide resolved
@nabetti1720
Copy link
Contributor Author

Commit#531dea3 caused overlapping prefixes that could not be removed and caused IO errors in the loader. I think I fixed it, but I am not sure if it was fixed correctly...

@nabetti1720
Copy link
Contributor Author

nabetti1720 commented Nov 12, 2024

@richarddavison , Following this fix: CJS export default (#673) , the following modules can no longer be loaded.

Specifically, the fix: CJS export default (#673) will cause an IO error when loading modules, and even if you fix it, an empty object will be returned.

// index17.js
const a = require('call-bind/callBound');
console.log(a)

Specifically, the "Fix default CJS exports” will cause an IO error when loading modules, and even if you fix it, an empty object will be returned.

Previous state (Commit#0a7509de):

% llrt index17.js
[function: callBind]

main branch (Commit#6dcd7784):

% llrt index17.js  
Error: IO Error: No such file or directory (os error 2)
  at <anonymous> (/Users/shinya/Workspaces/llrt-test/index17.js:1:11)

Commit#2d0e3381:

% llrt index17.js
{}

NOTE: Since this is not a regression generated by this PR, there is no problem in treating it as a separate issue.

@richarddavison
Copy link
Contributor

Specifically, the "Fix default CJS exports” will cause an IO error when loading modules, and even if you fix it, an empty object will be returned.

Fixed by https://github.com/awslabs/llrt/pull/674/files

@nabetti1720
Copy link
Contributor Author

nabetti1720 commented Nov 12, 2024

Specifically, the "Fix default CJS exports” will cause an IO error when loading modules, and even if you fix it, an empty object will be returned.

Fixed by https://github.com/awslabs/llrt/pull/674/files

Thank you very much. After rebese, the correction in this PR has been reverted.

tests/unit/assert.test.ts Outdated Show resolved Hide resolved
@richarddavison
Copy link
Contributor

Thank you. LGTM. Please rebase of main and i'll merge!

@nabetti1720
Copy link
Contributor Author

Thank you. LGTM. Please rebase of main and i'll merge!

This branch is already connected to the latest main branch. :)

@richarddavison richarddavison merged commit 77cf8ef into awslabs:main Nov 13, 2024
9 checks passed
@nabetti1720 nabetti1720 deleted the feat/assert-module branch November 13, 2024 21:46
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.

4 participants