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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support typescript config #110

Merged
merged 10 commits into from Dec 15, 2022
Merged

feat: support typescript config #110

merged 10 commits into from Dec 15, 2022

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 22, 2022

This is all pretty standard implementation wise but the types are not as exact as I'd like as it seems there's a difference between the types used by contentful itself & contentful-management that make them incompatible.

The beauty with this is that it's not an actual blocker, since we can improve the types over time 馃帀

(I suspect that this package should be sourcing it's types from contentful-management rather than contentful, if it's expecting the former to be being used to create the client, but that'd be a different change - I'm happy to do the work in a follow up PR if folks are happy with that change)

Resolves #33

@G-Rath
Copy link
Contributor Author

G-Rath commented Dec 7, 2022

@GabrielAnca any chance of getting this reviewed?

Copy link
Contributor

@GabrielAnca GabrielAnca left a comment

Choose a reason for hiding this comment

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

Hi @G-Rath 馃憢馃徎 Sorry for the delay, I was out of the office last week. Thanks a lot for this contribution. I'll be happy to help you get this merged into production! The PR looks great so far!

I tried this in one of our repos and it doesn't work, and the error seems to point that we're using different versions of typescript/ts-node

$ contentful-typescript-codegen --output @types/generated/contentful.d.ts
Error: Debug Failure. False expression: Non-string value passed to `ts.resolveTypeReferenceDirective`, likely by a wrapping package working with an outdated `resolveTypeReferenceDirectives` signature. This is probably not a problem in TS itself.

What versions of typescript and ts-node are you using? I'm using typescript 4.8.4 and ts-node 10.9.1

src/loadEnvironment.ts Show resolved Hide resolved
src/loadEnvironment.ts Outdated Show resolved Hide resolved
src/loadEnvironment.ts Outdated Show resolved Hide resolved
src/loadEnvironment.ts Show resolved Hide resolved
@G-Rath
Copy link
Contributor Author

G-Rath commented Dec 12, 2022

I tried this in one of our repos and it doesn't work, and the error seems to point that we're using different versions of typescript/ts-node
What versions of typescript and ts-node are you using? I'm using typescript 4.8.4 and ts-node 10.9.1

Are you sure you're using that version of ts-node? I've tried on our project using both TypeScript 4.5.4 and ts-node 10.4.0, and TypeScript 4.9.4 and ts-node 10.9.1 - both worked fine, but using TypeScript 4.9.4 with ts-node 10.4.0 did reproduce the error, which tracks with what I can find online about this error that indicates it was fixed in v10.6.0.

One thing in particular to watch out for is that you don't have multiple versions of ts-node in your tree, or that you're not depending on ts-node implicitly and that another package has it as a non-peer-dependency (which generally they shouldn't).

You can use yarn why ts-node and npm ls ts-node to verify the exact version(s) you have installed.

@G-Rath G-Rath requested review from GabrielAnca and removed request for GabrielAnca December 12, 2022 19:15
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@GabrielAnca GabrielAnca left a comment

Choose a reason for hiding this comment

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

I could connect the dots now, thanks for all the clarification! I made a couple small comments. As soon as they're addressed I'll be happy to merge this 馃槃 This will also improve our own setup a lot! Thank you so much!

@GabrielAnca
Copy link
Contributor

Looks great! Only one more thing: Let's also update the readme to document how to use a TypeScript file 馃槂

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@GabrielAnca GabrielAnca left a comment

Choose a reason for hiding this comment

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

This is great, thank you so much for all the effort here 馃殌馃殌馃殌

@GabrielAnca GabrielAnca merged commit 90c2a17 into intercom:master Dec 15, 2022
@GabrielAnca
Copy link
Contributor

馃帀 This PR is included in version 3.4.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@G-Rath G-Rath deleted the support-typescript-config branch July 6, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getContentfulEnvironment.js
2 participants