nodejs / import-in-the-middle

Like `require-in-the-middle`, but for ESM import
https://www.npmjs.com/package/import-in-the-middle
Apache License 2.0
72 stars 27 forks source link

ref: Vendor in module-details-from-path #120

Closed AbhiPrasad closed 4 months ago

AbhiPrasad commented 5 months ago

This PR vendors in module-details-from-path and rewrites it to match the syntax we use for import-in-the-middle.

I generally believe that less dependencies for diagnostics tooling is a good thing but there are other reasons to do this.

  1. Pulling this in means it becomes way easier for us to adjust it in the future, which I think we will do with the next couple of cleanups happening in the repo. I also hope that my extra comments helps make this easier to understand for external contributors in addition.
  2. Right now using module-details-from-path pulls in a couple of un-needed files (testing, travis config). We can alleviate this for the ~8.3 mil weekly downloads import-in-the-middle gets
image

One thing to note about the implementation is that I elected to add JSDoc based types. In my opinion adding a ts build process may be too heavy (unless we want to dual emit CJS/ESM from the lib), so I think this is a good middleground to get better intellisense while we are working on refactoring the lib. Happy to remove if folks thing this is unnecessary.

This PR came from me trying to reduce the import graph of a project that uses OpenTelemetry. Here's @opentelemetry/instrumentation for example.

timfish commented 4 months ago

Since this same package is used in require-in-the-middle too, this would result in more files/source for the majority of consumers.

AbhiPrasad commented 4 months ago

Yeah require-in-the-middle is a good point.

I wonder if module-details-from-path will accept some PRs then to atleast clean up the files thing. Let's try from that angle.

jsumners-nr commented 4 months ago

I don't see a reason to bother them with the maintenance burden. See https://github.com/fastify/skeleton/issues/42#issuecomment-1208090783