chore(gatsby): Convert cache.js to TypeScript #20622
Conversation
|
|
||
| get<T = unknown>(key): Promise<T | undefined> { | ||
| return new Promise(resolve => { | ||
| // eslint-disable-next-line no-unused-expressions |
There was a problem hiding this comment.
ESLint is complaining here, which I think is because it's an older version that doesn't recognize optional chaining
| @@ -1,5 +1,8 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "target": "ESNext", | |||
There was a problem hiding this comment.
I think this is the right target. I don't think it affects the actual compilation, as that's up to Bable. It's just for typechecking, so it knows which language features are available
| }, | ||
| { | ||
| store: this.store, | ||
| ttl: TTL, |
There was a problem hiding this comment.
cache-manager says this is a required proeprty, even though the fs-hash store uses the one inside options
| @@ -1,5 +1,8 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "target": "ESNext", | |||
| "lib": ["ES2015"], | |||
| "moduleResolution": "node", | |||
There was a problem hiding this comment.
Hey there! Can you explain these config changes to me?
There was a problem hiding this comment.
Sure thing.
targetEven though Babel chooses the actual compilation target, the type checker uses this to see if the target supports certain language features. I added this target because it defaults to ES3, which doesn't support getters and setters, so TS complains if you use them.lib. The default lib includes DOM, which is incorrect and was causing a conflic withCachemoduelResolution. Just telling it that we're using node. It was complaining about the json resolution option without it
| key: string, | ||
| value: T, | ||
| args: CachingConfig = { ttl: TTL } | ||
| ): Promise<T | undefined> { |
There was a problem hiding this comment.
We had a goal to avoid using generics as much as possible. Do you think it's possible to do here? I'm not totally familiar with the cache module yet. Is the value supposed to be any specific expected types? I would look through the codebase and see when we use this if we align on any specific types.
There was a problem hiding this comment.
Generics are unavoidable if you're typing Promises. I'm also using them for the cache getters and setters, because the return type depends on the input type.
| return new Promise(resolve => { | ||
| // eslint-disable-next-line no-unused-expressions | ||
| this.cache?.set(key, value, args, err => { | ||
| resolve(err ? undefined : value) |
There was a problem hiding this comment.
Can we use git mv to preserve git history? It's hard to review this file because I can't tell what's different. E.g., this resolve(undefined) on error feels like a bug.
There was a problem hiding this comment.
I'm a bit surpised it didn't pick up that this was a rename. That resolve(undefined) was in the original file.
| @@ -34,8 +34,7 @@ module.exports = { | |||
| `__tests__/fixtures`, | |||
| ], | |||
| transform: { | |||
| "^.+\\.js$": `<rootDir>/jest-transformer.js`, | |||
| "^.+\\.tsx?$": `<rootDir>/node_modules/ts-jest/preprocessor.js`, | |||
| "^.+\\.[jt]sx?$": `<rootDir>/jest-transformer.js`, | |||
There was a problem hiding this comment.
Thanks for jumping in here! Really love the community getting involved in this TS transformation. My main request at this oint is that we need to use git mv to change the file to ts instead of creating a new git file.
To fix this you should do this:
- Change file back to .js and commit it
- Use the command
git mv packages/gatsby/src/utils/cache.js packages/gatsby/src/utils/cache.tsand commit it
blainekasten
force-pushed
the
typescript-cache-js
branch
from
cf27849
to
e6399cc
Compare
ascorbic
force-pushed
the
typescript-cache-js
branch
from
66c891f
to
f668fc5
Compare
|
Replacing with #20626 |
Description
This includes the changes to enable Jest to run on TypeScript files that were in #20457. It also has a couple of additions to tsconfig to reflect the fact we're running in Node not the browser
Documentation
n/a
Related Issues
#20457