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

test(ext/node): Port _fs_writeFile_test.ts from deno_std #19524

Merged
merged 8 commits into from
Jun 19, 2023

Conversation

fbaltor
Copy link
Contributor

@fbaltor fbaltor commented Jun 15, 2023

Porting the node/_fs/_fs_writeFile_test.ts from deno_std, one of the tasks of the bigger issue #17840.

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2023

CLA assistant check
All committers have signed the CLA.

@fbaltor
Copy link
Contributor Author

fbaltor commented Jun 15, 2023

There are two main points I'm having difficult with, which are:

  1. The TextEncodings type, which is currently hardcoded in the test. The original test imports it from an utils file, but the correspondent one in the deno repo seems to be unreachable (ext/node/polyfills/_utils.ts, link).

  2. The test of cancellation with an AbortSignal (link) that is not passing due to the following error:

error: TS2719 [ERROR]: Type 'AbortSignal' is not assignable to type 'AbortSignal'. Two different types with this name exist, but they are unrelated.
image

@fbaltor fbaltor marked this pull request as ready for review June 15, 2023 22:23
@fbaltor fbaltor changed the title test(ext/node): Port node:fs writeFile and writeFileSync tests from deno_std test(ext/node): Port node:fs _fs_writeFile_test.ts from deno_std Jun 15, 2023
@fbaltor fbaltor changed the title test(ext/node): Port node:fs _fs_writeFile_test.ts from deno_std test(ext/node): Port _fs_writeFile_test.ts from deno_std Jun 15, 2023
writeFile(
"_fs_writeFile_test_file.txt",
value,
encoding as TextEncodings,
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you just skip as TextEncodings bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It complais: Argument of type 'string' is not assignable to parameter of type 'WriteFileOptions'. This parameter should be Encodings | CallbackWithError | WriteFileOptions | undefined, with type Encodings = BinaryEncodings | TextEncodings.

Copy link
Member

Choose a reason for hiding this comment

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

Then I guess as any will be required here as well

@bartlomieju
Copy link
Member

@fbaltor as for no 2, you will have to use as any there because of the problem described in #19527

@kt3k
Copy link
Member

kt3k commented Jun 16, 2023

@fbaltor Please run tools/format.js to pass CI checks

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 544878c into denoland:main Jun 19, 2023
@kt3k kt3k mentioned this pull request Jun 28, 2023
55 tasks
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