Adds missing exports (infamous triplet)

[fox1t]

As titled.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

lgtm

[mcollina]

@voxpelli can you confirm if this works for you?

[voxpelli]

It does not work

[mcollina]

Is Fastify working for you?

[fox1t]

@voxpelli can you use .default instead of the namespace one? That expression will never be callable since it is seen as ESM. You will have the same issue with complied TS that never exports a callable namespace.

[fox1t]

Is Fastify working for you?

In this scenario, .default is intended to be use: here we have a require call (that usually imports namespaces) that is requiring a faux ESM module as per TS types. Using .default should solve this problem.

[voxpelli]

Is Fastify working for you?

In this scenario, .default is intended to be use: here we have a require call (that usually imports namespaces) that is requiring a faux ESM module as per TS types. Using .default should solve this problem.

One shouldn't need to change ones code to get type validation working, if it runs, then the types shouldn't tell you that it won't run

[fox1t]

In TS isn't possible to achieve full compliance with all of the scenarios. As I said JS checked by TS is the only one we can't cover with this approach. All of the others, that are more used though, are covered and make the users happier.

[voxpelli]

JS checked with TS is one of the core scenarios and one which makes sense for Fastify itself as it is also a JS module.

One of the key wins of #16 compared to the fix here is that it actually checks that the types and the code matches: 694d09d

As this project is JS rather than TS, the only way to actually ensure that the types and the code matches is to use TS to validate the JS.

If any scenario shouldn't be supported, it's the old legacy pre-TS-2.7 non-esModuleInterop scenario, if all three can't be supported at once

[voxpelli]

I've added self-hosted types for the modules-playground/ fox1t/modules-playground#3 to allow experimentation and in that PR showed all cases that are supported with the triplet and all cases that are supported with the namespace solution

The ones who have esModuleInterop set to false can still use the namespace variant, they only need to include it as a cjs module:

import helloWorldRequire = require("../../")

[fox1t]

The problem is that some other packages, that our users depends on, use different esModuleInterop settings. This part of TS is a real mess because of the compiler checking also node_modules deps. We can't just "don't care about esModuleInterop".
Having this exports garantees us to support:

1. CJS
2. ESM in Node.js
3. TS esModuleInterop: true
4. TS esModuleInterop: false
5. JS checked by TS using .default prop. (faux module approach)

What is important to understand (I am repeating this as future reference too) is that if someone writes a TS package and export default (pretty much everyone) this will happen.

This TS export

export default moduleEntryPoint
export { moduleEntryPoint }

will be compiled to:

module.exports.default = moduleEntryPoint
module.exports.moduleEntryPoint = moduleEntryPoint

So also in this case const moduleEntryPoint = require('amazing-module') would not be callable.

Since we export .default always, you can const moduleEntryPoint = require('amazing-module').default and named export too, your specific case is covered: you can alsoconst { moduleEntryPoint } = require('amazing-module') if you prefer!

[fox1t]

I have to add that I perfectly understand your use case, and if there is any way to cover it without dropping the others, we will do our best to support it.
I am sorry that we broke this package for you: before today, we never check against JS checked by TS scenario. Thank you for pointing this out!

[voxpelli]

Yes, I'm not saying it isn't a breaking change, but as TS itself highly recommends one moving to esModuleInterop ("We highly recommend applying it both to new and existing projects.") while also steadily improving the support for JS itself its not a matter of if one should make it work plug and play with JS, but a matter of when.

TS has acknowledged that the pre-esModuleInterop behavior was a mistake, and all complexity rises from that mistake, so the sooner we leave that behind, the better.

As I've shown in fox1t/modules-playground#3 it is possible to get code to work in all using a non-triplet approach (but of course it would be breaking)

[voxpelli]

That said, of course this PR enables it to work at all (using the .default) but it still makes it so that anyone that introduces TS to validate their JS project will get a false negative unless they for some weird reason were already using .default.

So I guess this should be merged as it brings this project in line with the rest of the Fastify ecosystem, and then we can discuss a possible better next step in fox1t/modules-playground#3

[fox1t]

This is the real problem with your approach: https://github.com/fox1t/modules-playground/pull/3/files#diff-7fc4abcf18a5d2e9bec68d06cb32eaa705aaa9401e79fbb2a61361f8b91e390cR5

We are forcing our TS users to import the namespace even if we are exporting a perfectly valid faux module.

In addition, to thatimport = require is one of the other things that are strongly not recommended to be used anymore.

[voxpelli]

This is the real problem with your approach: https://github.com/fox1t/modules-playground/pull/3/files#diff-7fc4abcf18a5d2e9bec68d06cb32eaa705aaa9401e79fbb2a61361f8b91e390cR5

We are forcing our users to import the namespace even if we are exporting a perfectly valid faux module.

Lets have that discussion for that PR, lets merge this one here for now.

[voxpelli]

An alternative, even simpler, approach would be to just avoid a top-level export and only have named exports.

Only supporting:

const { envSchema } = require('env-schema');

And in TS:

import { envSchema } from "envSchema";

It's the module.exports = someFunction that has very poor compatibility with TS

[fox1t]

An alternative, even simpler, approach would be to just avoid a top-level export and only have named exports.

Only supporting:

const { envSchema } = require('env-schema');

And in TS:

import { envSchema } from "envSchema";

It's the module.exports = someFunction that has very poor compatibility with TS

Yes! I agrre with this and it is already supported! :) We stronly encourage all of our users to use named imports/requires. The namespace requires and imports are usual used by CJS users. Thats why we do always 3 exports: we cover all scenarios! :)

[voxpelli]

Right, maybe the other ways of importing it could be marked as deprecated then? That will actually get picked up by TS nowadays: https://devblogs.microsoft.com/typescript/announcing-typescript-4-0-beta/#deprecated-support

[voxpelli]

Could be achieved through:

declare function loadAndValidateEnvironment(
  _opts?: EnvSchemaOpt
): EnvSchemaData;

/** @deprecated */
declare function defaultExport(
  _opts?: EnvSchemaOpt
): EnvSchemaData;

export default defaultExport;
export { loadAndValidateEnvironment as envSchema };

[fox1t]

Deprecation is too strong for this case: it works perfectly in pure TS projects.
Let me recap all of the scenarios to make it more clear why we don't want to deprecate anything:

1. .default: implicitly used by TS projects and faux ESM (babel and bundlers)
   1.1 explicitly used as require('...').default by CJS checked by TS compiler
2. namespace export (only present in JS code and not in type definitions): CJS and Node.js ESM since Node.js v14
3. namedExport: used by CJS, TS, faux ESM, Node.js ESM (only in the very last Node.js version thanks to this: https://twitter.com/maksimsinik/status/1311268818040258561?s=20)

As you can see, all of our users can use the import that is most familiar with, and the one they expect can be used. The only real exception is point 1.1 that forces the explicit use of .default. Since it is the least used scenario, we are OK with this trade-off.

@mcollina and @Ethan-Arrowood, do you think it might be a good idea to write this recap somewhere to fastify team internal use?

[mcollina]

do you think it might be a good idea to write this recap somewhere to fastify team internal use?

I would prefer a public blog post or similar that we discuss publicly. This problem is really outside of my comfort zone.

[fox1t]

do you think it might be a good idea to write this recap somewhere to fastify team internal use?

I would prefer a public blog post or similar that we discuss publicly. This problem is really outside of my comfort zone.

I suppose I have something to write this weekend! :)