feat: detect Typescript typings for NPM modules and reference them from barrel files

[Skn0tt]

Closes https://linear.app/netlify/issue/COM-53/load-types-when-using-npm-modules-in-edge-functions.

When generating barrel files for NPM modules, type information can get lost. To prevent that, we're adding triple-slash typescript directive, that editors can use to resolve typing information. More info on that in the TypeScript docs.

I couldn't find a library to do this for us online, sadly :/ So I had to implement most of the logic myself. It's a bit convoluted, i'm very open to improvements here.

[Skn0tt]

making a for-loop out of this, so we can use await down below

[Skn0tt]

These will be absolute URLs. Should we change those to relative URLs instead?

[eduardoboucas]

I think it would be nicer, yes.

[Skn0tt]

Done in e4a82a7. Had to change servePath to a repo-local directory to test this.

[eduardoboucas]

We don't need to find the path to package.json, because NFT gives us this information. Every module's package.json is part of the reasons object, which you can identify with type: ["resolve"] or basename(path) === "package.json".

In fact, since every npm module will have a package.json, we can just discard any file that isn't a package.json for the purpose of finding direct dependencies (we still want to look at them for the purpose of finding extraneous files).

[Skn0tt]

I'm not so sure about this. Have we verified that NFT also includes the package.json for CJS modules? Or does it only include package.json if that contains "type": "module"?

[eduardoboucas]

Have we verified that NFT also includes the package.json for CJS modules?

Yes.

[Skn0tt]

I've tried implementing this, but I don't see how we can identify direct dependencies based on package.json. In the test case, here's how the reason for @pt-committee/identidade/package.json looks:

It references its internals, but not the user code that imports it. Since that's what we use to identify direct deps, we'd have to first detect the @pt-committee/identidade/index.js reason, and then find @pt-committee/identidade/package.json from that. That's tricky, and I prefer doing a (not strictly necessary) findUp instead. It's not very costly, only executed in local dev, and makes this easier to reason about.

If you seen an easier way of implementing this that i'm missing, please let me know / push a commit to this!

[eduardoboucas]

That's tricky, and I prefer doing a (not strictly necessary) findUp instead. It's not very costly, only executed in local dev, and makes this easier to reason about.

My issue with that is that if you have a module with 10,000 files, you'll be repeating this operation 10,000 times, with no caching whatsoever:

1. Traverse the module tree to find the package.json
2. Read the package.json from disk
3. Parse it
4. Look for types
5. Potentially traverse the module tree again to find @types/

It's not costly in the context of the synthetic test case we've put together here, but I don't think we can say the same when we're dealing with real modules.

[eduardoboucas]

On the flip side, you'd do:

1. Look at file: if it's not package.json, discard
2. Look up parent of package.json, if it's not a direct dependency, discard
3. Look up types (steps 2 to 5 above)

But only once for every node module that is a direct dependency.

[Skn0tt]

Look up parent of package.json, if it's not a direct dependency, discard

In order to properly detect direct dependencies, this means we need to look up the parents of package.json within NFT's reason object. Added a comment as to why in 444a78f. Assuming that's what you intend?

[eduardoboucas]

nit: Can we add a JSDoc comment here to explain what this does?

[Skn0tt]

done in 59d60b4

[eduardoboucas]

I think having the entire function wrapped in a try with nothing in the catch is a bit of an anti-pattern, because we're essentially swallowing every possible error that might occur regardless of who calls the function.

I understand we don't want to throw an error if anything goes wrong in this process, but I think it would be nicer if we left that decision up to the caller, because then it can also decide how to handle the failure (e.g. log, do nothing, etc.).

Have you considered making this function throw and move the try/catch to getNPMSpecifiers?

[Skn0tt]

Did that in 59d60b4. ESLint forced me to introduce a safelyDetectTypes function so that the logic isn't nested too deep.

[eduardoboucas]

Stuff for a future PR: it doesn't make a lot of sense to call these barrel files. At this point, when we create them, they are indeed barrel files, but further below after esbuild runs they will become the actual modules. It'd be great to name these with a slug of the module name and version.

[eduardoboucas]

Can we do this only when we're building for local development? We don't get any value from doing it for production builds and we'll pay the price of performance + adding a point of failure.

[Skn0tt]

done in 59d60b4

[eduardoboucas]

I think it would be nicer, yes.

[eduardoboucas]

I think this means we won't detect extraneous files? I think we can only discard a file that isn't package.json for the purpose of detecting direct dependencies, finding types, etc.

[Skn0tt]

Good point, yeah! I was assuming we have a test for that, but apparently we don't. Gonna add that later today.

[Skn0tt]

addressed in e206df4

[eduardoboucas]

Rather than finding node_modules and then trying to find the right file inside it, I wonder if we could do all that at once with something like:

const typesPackageJson = await findUp(`node_modules/${inferDefinitelyTypedPackage(packageJsonContents.name)}/package.json)`

I think that would simplify things a bit, but not a blocker.

[Skn0tt]

good idea! done in a0a27f0

[eduardoboucas]

nit

Suggested change

	* Starting from a `package.json` file, this tries detecting a typescript declaration file.
	* Starting from a `package.json` file, this tries detecting a TypeScript declaration file.

[Skn0tt]

fixed in a0a27f0

[eduardoboucas]

nit: I think the name of the method could be a bit more descriptive. "detect" doesn't tell me much about what this returns. I would consider calling this something like getTypesPath.

[Skn0tt]

renamed in a0a27f0

[eduardoboucas]

nit: I would call this getTypesPackageName. "Definitely typed" doesn't mean a lot to people who don't know that the repo where the types live have that name, so I think it could be a bit more descriptive.

Also you're calling this "types package" below, so it would make things a bit more consistent.

[Skn0tt]

renamed in a0a27f0

[eduardoboucas]

I find this variable name confusing. If I'm reading this right, the variable contains parents, not imports/children?

[Skn0tt]

renamed in a0a27f0

[eduardoboucas]

I know that this already existed in the code, but I don't think we need to call getPackageName again. We can just use packageName from above?

[Skn0tt]

done in a0a27f0

[eduardoboucas]

I really don't like that we are writing a file to disk, then loading it again into memory, and then writing it back to disk.

I think in the future it might be nice to set write: false in esbuild, so that we get the file contents directly and write to disk just once.

[eduardoboucas]

Could we create an issue so we do this as a follow-up?

[Skn0tt]

Done in a8880e0

[Skn0tt]

Addressed most of your feedback, working on the test case for npmSpecifiersWithExtraneousFiles now.

[eduardoboucas]

🚀

[eduardoboucas]

nit: This feels a bit odd. Could we not just use filePath?

[Skn0tt]

reverted this in 4483fe7

[eduardoboucas]

Could we create an issue so we do this as a follow-up?

[eduardoboucas]

It's really annoying that esbuild doesn't make it possible to preserve certain comments in the output (only if they follow the pattern of legal comments).

Otherwise it'd be super easy for us to add the comment to the barrel file and avoid getting in the business of re-writing files.

😞

[Skn0tt]

Yeah, also checked legal comments :/ The current way is pretty much the easiest, sadly ...

[eduardoboucas]

This is totally a nit, since you shouldn't have to follow any formatting conventions that aren't enforced by our Prettier/ESLint configurations. Here goes anyway, in the spirit of sharing feedback.

I find this function a lot harder to parse than it should be, mostly because the code is super compact. Having some whitespace in between the different "sections" of the logic would help, as would things like separating the condition from the statement in if clauses.

Also, if you had the package.json parsing and the @types/ parsing on two different functions, you could have much shorter variable names (e.g. packageJsonPath and packageJsonContents could simply be path and contents), which I think would also help.

Just a personal preference and definitely not a blocker, though.

[Skn0tt]

Agree, that makes things a lot easier. Thanks for the feedback! I guess I wanted to get this mergeable, so I didn't look too much at readability. Also, it's been a while since I've written PRs with more than 100 lines of new code 😅

[Skn0tt]

Done in aa604a8