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

Types overrides imports #488

Merged
merged 4 commits into from
Feb 8, 2023
Merged

Conversation

nick-keller
Copy link
Contributor

@nick-keller nick-keller commented Feb 4, 2023

Fix #480

Please review it carefully, I did not test it end to end.

Copy link
Owner

@adelsz adelsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @nick-keller!
I added some comments mainly regarding scope not being an enum and a few good to have ESM test cases.
Will you be open to adding some e2e test cases to packages/example?

docs-new/docs/typing.md Outdated Show resolved Hide resolved
docs-new/docs/typing.md Outdated Show resolved Hide resolved
packages/cli/src/declareImport.test.ts Show resolved Hide resolved
packages/cli/src/types.ts Outdated Show resolved Hide resolved
@nick-keller
Copy link
Contributor Author

Hey @adelsz,
I took your comments into consideration but I could not make the example work.

Running test gives me:

Step 2/6 : FROM $NODE_IMAGE
ERROR: Service 'build' failed to build : invalid reference format

And running check gives me:

error TS6059: File '/Users/nick/projects/pgtyped/packages/example/jest-cjs.config.ts' is not under 'rootDir' '/Users/nick/projects/pgtyped/packages/example/src'. 'rootDir' is expected to contain all source files.
  The file is in the program because:
    Matched by default include pattern '**/*'

error TS6059: File '/Users/nick/projects/pgtyped/packages/example/jest.config.ts' is not under 'rootDir' '/Users/nick/projects/pgtyped/packages/example/src'. 'rootDir' is expected to contain all source files.
  The file is in the program because:
    Matched by default include pattern '**/*'


Found 2 errors.

If you can help me make it work so that I can add examples, or add them yourself if you feel like it that;s be great!

@adelsz
Copy link
Owner

adelsz commented Feb 8, 2023

Thanks @nick-keller. The changes look good.
To run the example packages you need to specify the node version as follows NODE_VERSION=16 npm run test.
We run it as part of the CI https://github.com/adelsz/pgtyped/blob/master/.github/workflows/main.yml

@nick-keller
Copy link
Contributor Author

Sorry @adelsz but I cannot get it to work.
Running check still gives me the two errors I listed above. And running test kinda works until src/index.test.ts timesout. This is definitly not my cup of tea, and I don't have the skills and the patience to solve those kind of issues.

Furthermore:

  • The tests are not really checking that the generated types are correct. For instance expect(typeof dateAsString).toBe('string'); only checks that pg.types.setTypeParser(...) works, it is not checking that pgtyped actually typed the column as string.
  • In the package.json, dependencies are ^2.0.0, meaning that my work will not be reflected until you actually publish the package anyway.

I'll have to let you take it from here I'm afraid :/

@adelsz
Copy link
Owner

adelsz commented Feb 8, 2023

No worries, I can handle that on my side. Thanks for working on this major feature, it will be a great addition to pgTyped.

As for the issues you encountered, packages/example is one of the confusing parts of the codebase. I will have to write more detailed instructions in a CONTRIBUTING.md doc.

In case it might be of any future use I will comment on the issues you brought up:

The tests are not really checking that the generated types are correct. For instance expect(typeof dateAsString).toBe('string'); only checks that pg.types.setTypeParser(...) works, it is not checking that pgtyped actually typed the column as string.

The idea of the example package is to serve both as an e2e test (compile-time and runtime) and example usage of pgTyped.

The npm run test command does two things:

  1. Runs pgTyped, generating code for all queries. After running the type generation, a git diff will show if there are any unexpected problems with the generated queries and types (e2e compile-time test).
  2. Runs the generated queries as part of a Jest test suite. This ensures that the queries work at runtime.

In the package.json, dependencies are ^2.0.0, meaning that my work will not be reflected until you actually publish the package anyway.

pgTyped is a monorepo using npm workspaces, so running npm install actually crosslinks all of the pgTyped packages together locally (the exact versions in package jsons don't matter). Running npm run build builds all packages and makes them available to each other as if they were published.

As I said, all of this belongs in CONTRIBUTING.md doc, which has been long overdue.

@adelsz adelsz merged commit 8440d6a into adelsz:master Feb 8, 2023
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.

Improve typing customization
2 participants