[feat] Continuation of node fs polyfill with directory support

[cknight]

Continuation of the work for the fs polyfill in support of #3403. Specifically this adds internal implementations of the directory classes used in node and exported by yet to be implemented Deno fs polyfill methods fs.opendir(), fs.opendirSync(), or fsPromises.opendir().

[cknight]

Seems I've been caught out by the move to 'use strict' and a stale version of the VS Code plugin. I'll work through the issues raised.

[cknight]

@ry Any idea why the windows CI is failing with access denied issues for my new tests (using Deno.open()) while the ubuntu and macOS CI builds pass? I've not got a windows machine to test on.

Example failing output:

FAILED Closing current directory with callback is successful
PermissionDenied: Access is denied. (os error 5)
    at Object.constructError ($deno$/errors.ts:39:13)
    at unwrapResponse ($deno$/dispatch_json.ts:40:12)
    at Object.sendAsync ($deno$/dispatch_json.ts:91:10)
    at async Object.open ($deno$/files.ts:81:15)
    at async Object.fn (_fs_dir_test.ts:31:33)
    at async Object.runTests ($deno$/testing.ts:157:7)
    at async runTestModules (runner.ts:234:3)
    at async main (runner.ts:276:5)

Failing test for above output, failing on Deno.open(".") line:

test({
  name: "Closing current directory with callback is successful",
  async fn() {
    const fileInfo: Deno.File = await Deno.open(".");
    assert(Deno.resources()[fileInfo.rid]);
    let calledBack = false;
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    new Dir(fileInfo.rid, ".").close((valOrErr: any) => {
      assert(!valOrErr);
      calledBack = true;
    });
    assert(calledBack);
    assert(!Deno.resources()[fileInfo.rid]);
  }
});

These failures occur for both reading the current directory, e.g. . as above, and for a directory created by Deno.makeTempDirSync() in subsequent tests.

Also, as an aside, neither my VS Code nor tools/lint.py picked up on the new strict typescript rules. Running the cargo tests was the only way to discover these issues. Any ideas on how to get the new stricter rules showing in VS Code?

[ry]

Any idea why the windows CI is failing with access denied issues for my new tests

Not sure...

Also, as an aside, neither my VS Code nor tools/lint.py picked up on the new strict typescript rules. Running the cargo tests was the only way to discover these issues. Any ideas on how to get the new stricter rules showing in VS Code?

I'm not sure but I think VS Code / ESLint probably sniff for tsconfig.json and assumes non-strict when it doesn't find it. Maybe @kitsonk can give more clues.

[cknight]

OK, no worries, I'll dig around deeper and also take latest master as I'm seeing other reports of CI failures on Windows, maybe related.

[kitsonk]

I'm not sure but I think VS Code / ESLint probably sniff for tsconfig.json and assumes non-strict when it doesn't find it. Maybe @kitsonk can give more clues.

It would be up to whatever plugin to determine what to configure the language service yet. If it is @axetroy plugin, then I don't know if he has defaulted to strict mode now under Deno.

[axetroy]

Yes, if you enable the axetroy.vscode-deno extension, strict mode is enabled by default

https://github.com/axetroy/vscode-deno/blob/6ec1cb401f852ce71f3ebdb3e6fefe36d254d0b1/typescript-deno-plugin/src/plugin.ts#L18-L33

  private readonly DEFAULT_OPTIONS: ts_module.CompilerOptions = {
    allowJs: true,
    checkJs: false,
    strict: true,
    esModuleInterop: true,
    jsx: this.typescript.JsxEmit.React,
    module: this.typescript.ModuleKind.ESNext,
    moduleResolution: this.typescript.ModuleResolutionKind.NodeJs,
    outDir: "$deno$",
    resolveJsonModule: true,
    sourceMap: true,
    stripComments: true,
    target: this.typescript.ScriptTarget.ESNext,
    noEmit: true,
    noEmitHelpers: true
  };

[cknight]

Thanks both. I'm using the axetroy plugin, so will see if I can reproduce the missing strict behaviour I was seeing earlier and raise an issue if found.

[ecyrbe]

hi again, this one seems missing in your implementation :

• dir[Symbol.asyncIterator]()

[cknight]

Ah, nice spot, thanks. It was actually there, but under a different name while I was testing something. Now renamed as it should.

[cknight]

Well, that was a touch harder than I anticipated, but every day is a learning day :) Now ready for review.

[cknight]

@ry - This is now ready for review.

[cknight]

@ry - ready for review again. In the general, should I tag you (and/or others?) on pull requests ready for review? There's absolutely no rush with this one, but I'm unclear what the best process is once a PR is ready for review. Let me know what's best. Thanks!

[ry]

Looks good - just one comment...

(And yes, tagging me is a good way to get a review - sorry for the delay)

[cknight]

@ry link added and now ready for review again

[ry]

LGTM