Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): make definitions nodenext compatible #198 #199

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

cesarvspr
Copy link

Checklist

@cesarvspr
Copy link
Author

cc: @climba03003 @mcollina

test command:

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

types/FluentJSONSchema.d.ts Outdated Show resolved Hide resolved
@coveralls
Copy link

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 Coverage Status
Change from base Build 3327867208: 0.0%
Covered Lines: 403
Relevant Lines: 403

💛 - Coveralls

@coveralls
Copy link

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 Coverage Status
Change from base Build 3327867208: 0.0%
Covered Lines: 403
Relevant Lines: 403

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 28, 2022

Pull Request Test Coverage Report for Build 3346473469

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.

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 Coverage Status
Change from base Build 3327867208: 0.0%
Covered Lines: 403
Relevant Lines: 403

💛 - Coveralls

types/FluentJSONSchema.d.ts Outdated Show resolved Hide resolved
types/FluentJSONSchema.d.ts Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 21, 2022

@climba03003
PTAL. It should be now valid.

@Uzlopak Uzlopak linked an issue Nov 21, 2022 that may be closed by this pull request
2 tasks
@cesarvspr
Copy link
Author

thanks for this @Uzlopak

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

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 21, 2022

Can you provide a repo which i can clone?

@cesarvspr
Copy link
Author

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
Copy link
Contributor

greguz commented Jan 30, 2023

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

@aboutlo
Copy link
Collaborator

aboutlo commented Jan 30, 2023

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

@greguz
Copy link
Contributor

greguz commented Jan 31, 2023

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
Copy link
Contributor

Uzlopak commented Jan 31, 2023

@greguz
@aboutio

Maybe something like helmetjs/helmet#391

@aboutlo
Copy link
Collaborator

aboutlo commented Jan 31, 2023

@greguz without writing ALL the tests, can you check if it fixes the original issue: #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
Copy link
Contributor

Uzlopak commented Jan 31, 2023

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

@greguz
Copy link
Contributor

greguz commented Jan 31, 2023

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
Copy link
Contributor

greguz commented Jan 31, 2023

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
Copy link
Contributor

Uzlopak commented Jan 31, 2023

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

@greguz
Copy link
Contributor

greguz commented Jan 31, 2023

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
Copy link
Contributor

greguz commented Jan 31, 2023

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
Copy link
Member

Fdawgs commented Mar 10, 2023

@cesarvspr mind resolving the conflicts please?

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 13, 2023

@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
Copy link
Member

You can always map the type S and expose it.

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 13, 2023

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

@climba03003
Copy link
Member

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

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 13, 2023

Do you have some explicit change requests?

@climba03003
Copy link
Member

climba03003 commented Mar 13, 2023

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to import in TS project with "moduleResolution": "NodeNext",
7 participants