Fix and improve type definitions

[voxpelli]

Here's a couple of fairly atomic commits that builds on top of the types that was merged in #14.

I had issues when using the types added in #14. They required me in my CommonJS requires to reference a non-existing .default attribute rather than using it as intended.

Rather than freestyling, I just frankly copied another approach from @types/deep-freeze, which worked well for me.

This PR contains:

• Types that are compatible with both CommonJS and ESM
• Tests that ensures that the above is the case
• Tests that ensures that the code and the types actually agree with one another
• Tests that ensures that the options schema definition and the options type definition actually are compatible with one another
• An off-topic change that adds a target version in package.json – I hope that can be okay 🙏

I would love to work with you on any feedback or changes to this 👍

[mcollina]

All the other modules in this organization follows the same style with tsd. I would prefer to avoid such a significant rewrite you are proposing here.

I think your problem was extensively researched by @fox1t and we just need to add a .default property in the current index.js to make this work.

See https://github.com/fastify/fastify/blob/d35a0fb66bca470ebfadca9dafd8c626a324fa9d/fastify.js#L610-L623 for some details.

[voxpelli]

Most of the tsd changes are because the way they was structured previously didn’t really test anything. It created a variable of a specific type then tested whether the variable was of that type, rather than actually testing whether a certain value was assignable to a certain type. I think the solution to the types you point to wouldn’t be the right one here, it would mean that I would have to indeed import a “default” property rather than requiring it normally, and the types that I have set up here should provenly works for all cases? (Unless I’ve missed some case in the tests) Though: I do admit that this is a complicated topic. I think the two of us has discussed it on Twitter as well some weeks or months ago, it is indeed not the easiest of things. I would very much like to help dig further into this. Maybe I could remove some of the non-core things of this PR to slim it down and we can maybe focus on getting a better grasp on that core? tors 15 okt. 2020 kl. 23:45 skrev Matteo Collina <[email protected]>:

…

All the other modules in this organization follows the same style with tsd. I would prefer to avoid such a significant rewrite you are proposing here. I think your problem was extensively researched by @fox1t <https://github.com/fox1t> and we just need to add a .default property in the current index.js to make this work. See https://github.com/fastify/fastify/blob/d35a0fb66bca470ebfadca9dafd8c626a324fa9d/fastify.js#L610-L623 for some details. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAINGKFYHMZXU5RNASANUDSK5UODANCNFSM4SSLJXXA> .

[mcollina]

You can see the bulk of the research here: https://github.com/fox1t/modules-playground. Essentially adding a default property fixes all possible cases. I'm not really an expert on this and I'm mostly relying on what @fox1t stated which works well for the rest of the projects in this org. Between cjs, esm, faux modules and typescript various options, it is indeed a mess.

I agree that breaking this into small improvements / separate PRs would allow us to make quick progress.

[mcollina]

I think these types are not correct btw, they need to follow a different approach, with namespaces etc. This is similar to what we need to do it here: https://github.com/mercurius-js/mercurius/blob/7e8ea6894bed2918eb19299e0529696624c94e3c/index.d.ts#L251.

[voxpelli]

Added a missing case fox1t/modules-playground#2 which is the same case that I'm trying to fix here, so I guess we'll see what happens in that repo and try to solve things there before moving on here. Sounds like a good plan?

[fox1t]

This is the same old problem :)

We can support only a subset of possible combinations. One of the biggest problems is this: https://github.com/fastify/env-schema/pull/16/files#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8R1
This is how TS represents namespace exports for CJS context.

However, in fastify organization, we decided to use the "infamous triplet" in order to support default, namespace, and named imports in:

• CJS context
• TS context
• ESM context
  You can find it here: https://github.com/fastify/fastify/blob/master/fastify.js#L621
  In our types we export "only" default(https://github.com/fastify/fastify/blob/master/fastify.d.ts#L48) and named (https://github.com/fastify/fastify/blob/master/fastify.d.ts#L136). The namespace export is completely missing.

However, since we write JS we need to add manually the proper typings to almost every package we author. We deliberately choose to represent types only as faux modules because it allows us to be independent of the tsconfig.json and babel/webpack configurations in almost 100% of the cases.

Let's try to figure out how can we handle this. :)

[voxpelli]

@fox1t Is there any scenario which the types I propose here doesn't support which the "infamous triplet" do support?

[fox1t]

The biggest issue with export = is how it reacts to esModuleInterop setting.

Unfortunately if the user uses a different setting in its tsconfig.json it can't use any package that was authored using the opposite setting. The only solution should be setting skipLibCheck to true but the users usually doesn't want to do that :(

[fox1t]

I had issues when using the types added in #14. They required me in my CommonJS requires to reference a non-existing .default attribute rather than using it as intended.

If this happens that there is a bug in JS code inside this package!

[fox1t]

We have a bug here: https://github.com/fastify/env-schema/blob/master/index.js#L81
The triplet is missing!

[fox1t]

To recap what is going here. We are facing for the first time a new scenario: JS code checked by TS compiler! 🎉

We need to figure out:

1. do we need to support also this new case?
2. if so, is the infamous triplet robust enough?

@mcollina @delvedor

[fox1t]

I've opened a PR to fix the missing exports issue: #17

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.