ref(nextjs): Prework for wrapping data-fetching functions #5508
Conversation
| @@ -0,0 +1,10 @@ | |||
| // TODO Use real webpack types | |||
| export type LoaderThis<Options> = { | |||
There was a problem hiding this comment.
Totally optional but we could type this as { addDependency: blah } & ({ query?: blah } | { getOptions?: blah }) so we have the "either-or" nature of this type baked in.
There was a problem hiding this comment.
Good idea. Will do.
UPDATE: Turns out TS can't handle it, at least not gracefully. Doing that make lines like
const { distDir } = this.getOptions ? this.getOptions() : this.queryerror out with
Property 'getOptions' does not exist on type 'LoaderThis<LoaderOptions>'.
Property 'getOptions' does not exist on type '{ query: LoaderOptions; } & { addDependency: (filepath: string) => void; }'
Annoying that it won't let you even check to see if something is there, but not sure it's worth trying to convince it otherwise.
There was a problem hiding this comment.
Ah right should have tried it out - my bad. I think you need to do the check something like 'getOptions' in this ? ... : .... But doesn't really matter.
There was a problem hiding this comment.
Ah, but then you run into microsoft/TypeScript#21732. (Specifically - switching to 'getOptions' in this ? this.getOptions() : this.query results in Cannot invoke an object which is possibly 'undefined'. (about this.getOptions()), while still leaving you with the error above with respect to this.query.)
TS - can't live with it, can't live without it.
| // Regardless of whether nextjs is using the CJS or ESM version of our SDK, we want the code from our templates to be in | ||
| // ESM (since we'll be adding it onto page files which are themselves written in ESM), so copy the ESM versions of the | ||
| // templates over into the CJS build directory. (Building only the ESM version and sticking it in both locations is | ||
| // something which in theory Rollup could do, but it would mean refactoring our Rollup helper functions, which isn't | ||
| // worth it just for this.) |
There was a problem hiding this comment.
I think this is the kind of technical debt we will never get rid of. Let's leave it but take the learning that DRY has it's downsides in regards to rollup configs.
There was a problem hiding this comment.
Interesting - not sure I'd call this tech debt. This is a weird one-off, and I feel like quite often in those cases the answer isn't "make your thing (whatever it is) support the uncommon edge case" it's "find a workaround," which to me is exactly what this is, no?
This is a number of boring structural changes pulled out of #5503 in order to make it easier to review.
Included changes:
LoaderThistype into a generic, so that each loader can specify its own options but share everything specified by webpack.