fix(types): make definitions nodenext compatible #198

[cesarvspr]

Checklist

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

[cesarvspr]

cc: @climba03003 @mcollina

test command:

tsc types/FluentJSONSchema.test-d.ts --module 'nodenext'

[coveralls]

Pull Request Test Coverage Report for Build 3341861259

• 0 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 3327867208:	0.0%
Covered Lines:	403
Relevant Lines:	403

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 3341861259

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 3327867208:	0.0%
Covered Lines:	403
Relevant Lines:	403

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 3341861259

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 3327867208:	0.0%
Covered Lines:	403
Relevant Lines:	403

---

💛 - Coveralls

[Uzlopak]

@climba03003 PTAL. It should be now valid.

[cesarvspr]

thanks for this @Uzlopak

but still seems to be not working with "type":"module" on the package.json as raised here

[Uzlopak]

Can you provide a repo which i can clone?

[cesarvspr]

Can you provide a repo which i can clone?

https://github.com/cesarvspr/issue-reproduction don't forget to replace the type definition inside node modules to have this branch implementation

[greguz]

Any update about this PR? May I help with something to speed-up those changes?

[aboutlo]

It would be great to test it, it's unclear if it's really fix the root issue

[greguz]

I'm writing a "test all the possible TypeScript craziness" project to test any combination between:

• set of TS compiler options
• different TS versions
• different style of imports (different scripts)

I hope to have some results soon. It will be published on my GitHub.

[Uzlopak]

@greguz @aboutio

Maybe something like https://github.com/helmetjs/helmet/pull/391

[aboutlo]

@greguz without writing ALL the tests, can you check if it fixes the original issue: https://github.com/fastify/fluent-json-schema/issues/198. Of course, if you want to add more test coverage is SUPER welcome, but I'm saying this in the interest of containing the scope

[Uzlopak]

Something like what i did for helmet should be easily implemented.

[greguz]

I'm not entirely sure how to write TS tests with multiple tsconfig.json, package.json, different import styles, etc (and I'm also not sure if It can be useful).

Just to be sure (because with TypeScript anything is possible), I've created a project that transpiles, tests, and execute some scripts against this project.

I added the patch contained inside this PR, but inside the reports dir I see some unexpected errors. Like this one.

[greguz]

The types update specified in this PR are not compatible with NodeNext/Node16 modules after the TypeScript 1.8 "patch". This is because of how the namespace is handled by TypeScript within the "new" module system. I've found a way to improve types compatibility, but there's a problem with S.null() export.

This file passed almost any configured environment combination. The main difference is the removal of namespace wrapper and the addition of named exports for all root methods (.object(), .string(), etc). The only problem is the reserved keywork null that makes not possible to export the .null() method.

[Uzlopak]

@climba03003 Please have a look. I used your schema for nodenext.

[greguz]

I used the two types of import described inside the docs:

import S from 'fluent-json-schema'

and

import * as S from 'fluent-json-schema'

Other ways of import (like import { default as S } from 'fluent-json-schema') were not tested.

[greguz]

I've missed some exports, and sadly I've found more than one problem with some reserved keywords:

export declare const null: S['null']
export declare const enum: S['enum']
export declare const const: S['const']
export declare const default: S['default']

Those exports throws an error because the usage of some reserved keyword. All other exports are ok.

I start to think that the only way to fix this issue entirely with Node16/NodeNext modules is to add/migrate to import { S } from 'fluent-json-schema' syntax.

[Fdawgs]

@cesarvspr mind resolving the conflicts please?

[Uzlopak]

@Fdawgs

This PR got kind of overridden by #207. @greguz was very effective in proposing his solution, despite we had the "template" of @climba03003 and are using it in this PR.

Basically resolving the merge conflicts is the same as reverting the solution in #207.

So the question is: What is the preferred solution? This or #207

[climba03003]

You can always map the type S and expose it.

[Uzlopak]

You can have only one export, when we want to use export = notation

[climba03003]

Nope, you can export inside the namespace. It does the same effect when you import as named type.

[Uzlopak]

Do you have some explicit change requests?

[climba03003]

Do you have some explicit change requests?

No, currently. I see the S is an interface. Only afraid if anyone using it as variables may break (not tested but high possibility)