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

bug: fix usage of * in type on Windows #1942

Merged
merged 4 commits into from May 19, 2024
Merged

Conversation

SampsaKaskela
Copy link
Contributor

Fixes #1941

@SampsaKaskela
Copy link
Contributor Author

SampsaKaskela commented May 15, 2024

Do you have idea what might be happening with Windows tests? I tried to run this on my PC and everything passed. It seems that the minify tests are failing and there seems to be with keyword being used in dist files but when I look at dist folder on my PC there is assert keyword being used. What could be the difference? I tried to change assert to with and tests still seemed to pass on my PC.

@domoritz
Copy link
Member

Hmm, could this be a babel issue?

@SampsaKaskela
Copy link
Contributor Author

I think I might have figured it out. The ubuntu-latest is running Node 18.20.2 while Windows test is running 18.18.2. It seems that 18.18.2 doesn't support with keyword which causes tests to fail. Support for it was added in 18.20.0. Would you be able to update Node version in Windows tests?

@domoritz
Copy link
Member

You should be able to set the version in the tests yourself.

@arthurfiorette
Copy link
Collaborator

arthurfiorette commented May 17, 2024

I could also reproduce this bug on ubuntu when trying to run ts-json-schema-generator on itself.

Firstly, comment this ts export inside index.ts:

//import ts from "typescript";
//export { ts };

then run:

yarn build
node bin/ts-json-schema-generator.js -p index.ts -f tsconfig.json -t '*'
# {
#   "$schema": "http://json-schema.org/draft-07/schema#",
#   "definitions": {}
# }

my versions:

{
  'ts-json-schema-generator': '2.0.0',
  npm: '10.8.0',
  node: '20.13.1',
  acorn: '8.11.3',
  ada: '2.7.8',
  ares: '1.28.1',
  base64: '0.5.2',
  brotli: '1.1.0',
  cjs_module_lexer: '1.2.2',
  cldr: '45.0',
  icu: '75.1',
  llhttp: '8.1.2',
  modules: '115',
  napi: '9',
  nghttp2: '1.61.0',
  nghttp3: '0.7.0',
  ngtcp2: '1.1.0',
  openssl: '3.0.13+quic',
  simdutf: '5.2.4',
  tz: '2024a',
  undici: '6.13.0',
  unicode: '15.1',
  uv: '1.46.0',
  uvwasi: '0.0.20',
  v8: '11.3.244.8-node.20',
  zlib: '1.3.0.1-motley-7d77fb7'
}

this current PR does not fixes it

@SampsaKaskela
Copy link
Contributor Author

@arthurfiorette I'm not sure if this is withing scope of this issue but I could try to take a look since I can produce your result in Windows too although it seems this is completely different problem than what I discovered.

@SampsaKaskela
Copy link
Contributor Author

@arthurfiorette So it seems that when using * then the code doesn't really dig deeper in import tree if you just re-export and don't import and use the types in target files. For example if I create test.ts and test1.ts and then make test.ts just to export everything from test1.ts then I will get empty definitions. Maybe this is intentional behavior that it doesn't go deeper than what it needs to complete the types defined in target files. You could just use src/**/*.ts to get the desired result instead of index.ts. I don't think I will try to fix this because at least I personally find it neat that you can limit what types it exactly gets instead of it getting all the types deeper in tree even if you don't use them in target files.

@arthurfiorette
Copy link
Collaborator

arthurfiorette commented May 18, 2024

I think it's indeed a bug. I traced it and ended up in this method:

protected inspectNode(node: ts.Node, typeChecker: ts.TypeChecker, allTypes: Map<string, ts.Node>): void {

The switch statement doesnt cover all Export kinds.

I could try to code a PR, what do you guys think?

@domoritz
Copy link
Member

PRs are lovely.

@SampsaKaskela
Copy link
Contributor Author

Could I get some help with codecov/project test? I don't exactly know what I should do to fix it.

@domoritz
Copy link
Member

Codecov doesn't need to pass but would be nice to have changes covered by a test.

@SampsaKaskela
Copy link
Contributor Author

I think the windows test already covers this change. If this change was not implemented then windows tests would fail and failing tests seem to precisely be about it creating empty definitions in this case.

@domoritz domoritz merged commit 3d625a9 into vega:next May 19, 2024
3 of 4 checks passed
arthurfiorette pushed a commit to kitajs/ts-json-schema-generator that referenced this pull request May 22, 2024
* bug: fix usage of * in type on Windows

* Run prettier

* ci: test windows

* ci: set node version to tests

---------

Co-authored-by: Dominik Moritz <domoritz@gmail.com>
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.

V2 creates empty definitions after updating from V1
3 participants