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(std/node): Add base64url encoding support, indexOf, lastIndexOf and includes to Buffer #1636

Merged
merged 13 commits into from Nov 29, 2021

Conversation

Soremwar
Copy link
Contributor

No description provided.

@Soremwar Soremwar changed the title fix(std/node): Add base64url encoding support, indexOf, lastInbdexOf and includes fix(std/node): Add base64url encoding support, indexOf, lastIndexOf and includes to Buffer Nov 27, 2021
@@ -68,6 +68,7 @@
"test-buffer-failed-alloc-typed-arrays.js",
"test-buffer-fakes.js",
"test-buffer-from.js",
"test-buffer-indexof.js",
Copy link
Member

Choose a reason for hiding this comment

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

nice 👍

@@ -37,7 +37,7 @@ import {
validateAbortSignal,
validateBoolean,
validateFunction,
} from "./_validators.ts";
} from "./internal/validators.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this renames might seem unnecessary, but this actually solves two problems:

  1. _validators is actually a shim of internal/validators however it was left in that location in the early days of std/node, now it will be easier to find the required functions when porting modules
  2. This solves a circular dependency created between _validators and internal/validators by moving the content of _validators

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, it was bothering me too; good change.

@@ -5,10 +5,10 @@
// Taken from Node 16.13.0
// This file is automatically generated by "node/_tools/setup.ts". Do not modify this file manually

'use strict';
"use strict";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why this formatting wasn't being applied, all I did was run setup.ts and all this changes were applied

@Soremwar
Copy link
Contributor Author

Ready for review. Since this apparently also formatted some files unintentionally I gotta point out the relevant files:

  • node/_buffer.js
  • node/_tools/config.json
  • node/_tools/suites/parallel/test-buffer-includes.js
  • node/_tools/suites/parallel/test-buffer-indexof.js
  • node/internal/buffer.js
  • node/_validators.ts
  • node/internal/validators.js
  • node/internal_binding/_node.ts
  • node/internal_binding/_utils.ts
  • node/internal_binding/buffer.ts
  • node/internal_binding/string_decoder.ts

@bartlomieju
Copy link
Member

Since this apparently also formatted some files unintentionally I gotta point out the relevant files:

Any idea why this happened? Changed files have // deno-fmt-ignore-file directives and shouldn't be touched by formatter. Did you use _tools/format.js script to format the PR? These files should be reverted before landing the PR.

@Soremwar
Copy link
Contributor Author

I have no idea what's going on :/

It doesn't seem to be deno fmt though, cause when I run it directly over the file after restoring it it doesn't affect it at all, but if I run setup.ts the changes resurface. Which makes no sense because the original format is different than the one we end up with

@Soremwar
Copy link
Contributor Author

Ok, I found out what was going on. Apparently I ran deno fmt and it formatted the cached decompressed versions of the file as well. Should be fine now

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, thank you @Soremwar

@bartlomieju bartlomieju merged commit 0628e44 into denoland:main Nov 29, 2021
@Soremwar Soremwar deleted the buffer branch November 29, 2021 18:36
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.

None yet

3 participants