Add file caching utility function #20

mcmire · 2023-10-26T22:23:08Z

One of the things that this tool will do is to hit the GitHub API and request all of the repositories under the MetaMask organization. We don't want to do this every time the tool is run, though, or else we might get rate limited. To prevent this, we can cache the response data from the API in a file.

This fetchOrPopulateFileCache function being introduced here (which we will use in a later commit) makes that possible.

Related to #5.

See here for context on how this function will eventually be used:

https://github.com/MetaMask/module-lint/blob/add-initial-code/src/ensure-metamask-repositories-loaded.ts#L39

socket-security · 2023-10-26T22:24:02Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/utils	8.2.0	filesystem	`+17`	4.96 MB	metamaskbot

One of the things that this tool will do is to hit the GitHub API and request all of the repositories under the MetaMask organization. We don't want to do this every time the tool is run, though, or else we might get rate limited. To prevent this, we can cache the response data from the API in a file. This `fetchOrPopulateFileCache` function being introduced here (which we will use in a later commit) makes that possible.

mcmire · 2023-10-26T23:01:56Z

tsconfig.json

@@ -9,6 +9,9 @@
    "noEmit": true,
    "noErrorTruncation": true,
    "noUncheckedIndexedAccess": true,
+    "paths": {
+      "@metamask/utils/node": ["./node_modules/@metamask/utils/dist/types/node"]


This is here because we've instructed TypeScript to use its "classic" module resolution algorithm via moduleResolution = node (and we're doing that because it works best with the way that the extension compiles JavaScript). Unfortunately, this means that TypeScript is not able to resolve imports for @metamask/utils/node. This is a special way of using the @metamask/utils package that grants access to Node.js-specific functions (added recently). The @metamask/utils/node path works because @metamask/utils defines an exports field in its manifest: https://github.com/MetaMask/utils/blob/fb9440203099538670a4af95da16265f878f2cc9/package.json#L30. By using paths, we are working around the limitation by mapping @metamask/utils/node to dist/types/node.d.ts inside of @metamask/utils.

Unfortunately, making this setting prohibits this package from ever being used as a library. At the moment, this is okay, because we're only using it as a CLI, which means it runs in Node, and Node knows how to read exports fields. But if we ever do want to expose an API, we'll have to come up with a better solution. Actually, we can still use this package as a library in the future. We only expect people will use this package in Node, and Node has supported the exports field natively for a while.

and we're doing that because it works best with the way that the extension compiles JavaScript

Could you elaborate on this part? I'm curious to know what the incompatibility is here. Using modern module resolution would simplify things

Sorry, that doesn't make sense. I was really trying to say "it works best with how we compile and use libraries in general".

First, if we change moduleResolution to node (and just this, nothing else), this will not work once we start using TypeScript 5.2, which revised the rules around module and moduleResolution such that moduleResolution = nodenext must be paired with module = nodenext.

When I do this on another another branch that compiles all of my changes for this project into one and run yarn build, I get:

node_modules/@metamask/utils/dist/types/assert.d.ts:1:15 - error TS2305: Module '"superstruct"' has no exported member 'Struct'. 1 import type { Struct } from 'superstruct'; node_modules/@metamask/utils/dist/types/assert.d.ts:1:29 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("superstruct")' call instead. 1 import type { Struct } from 'superstruct'; ~~~~~~~~~~~~~ ... node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path. 1 export * from './error';

Hmm OK, I'd like to dig into this further but it doesn't need to block this PR

Gudahtt · 2023-11-09T17:01:45Z

jest.config.js

@@ -22,7 +22,7 @@ module.exports = {
  collectCoverage: true,

  // An array of glob patterns indicating a set of files for which coverage information should be collected
-  collectCoverageFrom: ['./src/**/*.ts'],
+  collectCoverageFrom: ['./src/**/*.ts', '!./src/logging-utils.ts'],


Hmm, this should be covered just via it being imported. Any reason to exclude it?

Gudahtt · 2023-11-09T17:36:27Z

src/fetch-or-populate-file-cache.ts

+ * the file.
+ *
+ * @param args - The arguments to this function.
+ * @param args.filePath - The path to the file where the data should be saved.


Nit: Perhaps we could update these docs, or the parameter name (or both) to make it more clear that this is the path to the cache file? When I first read this, I thought this was how the data was output (e.g. that this was a "download data and write to this file" function.

Gudahtt · 2023-11-09T17:39:18Z

src/fetch-or-populate-file-cache.ts

+  const now = new Date();
+
+  if (await fileExists(filePath)) {
+    const cache = await readJsonFile<FileCache<Data>>(filePath);


Nit: Have you considered trying to read the file directly and handling the "does not exist" error, rather than checking for existence first? That would align better with what the Node.js docs recommend:

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

Gudahtt · 2023-11-09T17:46:13Z

src/fetch-or-populate-file-cache.test.ts

+
+  describe('if the given file already exists', () => {
+    describe('and no explicit max age is given', () => {
+      describe('and it was created less than an hour ago', () => {


Nit: Have you considered parameterize these tests? We might be able to cut down on the repetition that way. These all look like they're testing the same thing with different inputs.

Or we could make the max age not configurable. Not sure how likely it is that we'll want to set that in practice

Gudahtt

LGTM! I can re-review once conflicts are resolved

mcmire · 2023-11-12T18:49:23Z

This PR isn't needed anymore. I was going to use it to cache the call to the GitHub API which fetches the list of MetaMask repositories, but it turns out that gh can do this already, so no sense in reimplementing this (at least if/when we need to).

mcmire requested a review from a team as a code owner October 26, 2023 22:23

mcmire force-pushed the fetch-or-populate-file-cache branch from 1f50693 to 148374b Compare October 26, 2023 22:53

mcmire force-pushed the fetch-or-populate-file-cache branch from 148374b to 9e5b19c Compare October 26, 2023 22:54

mcmire commented Oct 26, 2023

View reviewed changes

mcmire mentioned this pull request Oct 26, 2023

Add RepositoryFilesystem #21

Merged

Gudahtt linked an issue Nov 9, 2023 that may be closed by this pull request

Add file caching utility function #27

Closed

Gudahtt reviewed Nov 9, 2023

View reviewed changes

mcmire closed this Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file caching utility function #20

Add file caching utility function #20

mcmire commented Oct 26, 2023 •

edited

Loading

socket-security bot commented Oct 26, 2023

mcmire Oct 26, 2023 •

edited

Loading

Gudahtt Nov 6, 2023

mcmire Nov 7, 2023 •

edited

Loading

Gudahtt Nov 9, 2023

Gudahtt Nov 9, 2023

Gudahtt Nov 9, 2023

Gudahtt Nov 9, 2023

Gudahtt Nov 9, 2023

Gudahtt Nov 9, 2023

Gudahtt left a comment

mcmire commented Nov 12, 2023

Add file caching utility function #20

Add file caching utility function #20

Conversation

mcmire commented Oct 26, 2023 • edited Loading

socket-security bot commented Oct 26, 2023

mcmire Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Nov 6, 2023

Choose a reason for hiding this comment

mcmire Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt Nov 9, 2023

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

mcmire commented Nov 12, 2023

mcmire commented Oct 26, 2023 •

edited

Loading

mcmire Oct 26, 2023 •

edited

Loading

mcmire Nov 7, 2023 •

edited

Loading