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(node/crypto): Stub out missing exports #2263

Merged
merged 20 commits into from
May 24, 2022

Conversation

cmorten
Copy link
Contributor

@cmorten cmorten commented May 23, 2022

Stub out the remainder of the crypto module with the correct interfaces, invoking notImplemented() if called.

With the inclusion of the browserify directory into deno_std I suspect there are some methods which could already be implemented, but to narrow scope here I have opted for using notImplemented() more often than not.

I have restructured the crypto implementation somewhat to align with the Node directory structure ( see #1707 ) but have left the crypto_browserify directory where it was as moving results in a ~250 file change which is probably best done as a chore in a separate PR to make for easier reviewing.

This structural PR should hopefully pave the way to make implementing the following easier:

return (
obj != null && (obj as Record<symbol, unknown>)[kKeyObject] !== undefined
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file avoids circular deps issues.

@cmorten cmorten marked this pull request as ready for review May 24, 2022 09:11
@cmorten cmorten requested review from bartlomieju and kt3k as code owners May 24, 2022 09:11
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@cmorten Thanks for doing this! Left one comment

node/internal/crypto/cipher.ts Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kt3k kt3k merged commit f547f5e into denoland:main May 24, 2022
@cmorten cmorten deleted the feat/node-crypto-stub-interfaces branch May 24, 2022 14:41
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