contentful-migrate icon indicating copy to clipboard operation
contentful-migrate copied to clipboard

Typescript migrations

Open shennyg opened this issue 5 years ago β€’ 10 comments

It looks like https://github.com/tj/node-migrate/pull/144 needs to be merged in before we can get typed migrations working.

shennyg avatar Nov 04 '19 17:11 shennyg

I'll have to spend some time experimenting with this. Reading the above PR, and its associated issues, seems that there are alternative ways for implementing TS migrations

deluan avatar Nov 08 '19 15:11 deluan

@deluan Any interest in / openness to a PR that provides a workaround for using TypeScript?

Pros:

  • Gets TypeScript migrations working.
  • Fairly uninvasive: adds a flag to each relevant command (AFAICT: just list, up, down) and delegates to the existing registerCompiler stuff in tj/node-migrate) for the actual work.

Cons:

  • Requires users to add a file like the one here, and point to it from the command-line argument. We could alternatively make the contentful-migrate flag be from more of a closed set to make it user-friendlier (e.g. something like --compiler typescript), but I'm more inclined to leave it open to extension in letting more options besides typescript potentially work.
  • May mean breaking changes if/when tj/node-migrate changes their API as described in the above ticket, although they're already thinking/talking about handling breaking changes with major version bumps.
  • Opens up questions about whether the migration codegen should support TypeScript.

Thoughts? I'd love to be using TypeScript, but this tool seems like a must for Contentful migrations. Nice work!

trptcolin avatar Feb 06 '20 22:02 trptcolin

Hey folks – any updates on this? I'm very interested in getting TS migrations supported πŸ˜„ πŸ˜…

devlato avatar Jun 26 '20 12:06 devlato

In meanwhile, I forked this repo to add support for custom templates (with --migration-template option/-t option/TEMPLATE_FILE env variable) and custom generated file extensions (with --extension option/-e option/MIGRATION_FILE_EXTENSION env variable) and published a library fork named https://www.npmjs.com/package/contentful-migrate-fork. Additionally, I've created a PR for merging the changes into the original library.

devlato avatar Jun 26 '20 13:06 devlato

+1 for Typescript support :)

nicam avatar Oct 30 '20 10:10 nicam

Is there a level of simplified PR that you're likely to have time to review @deluan to make this package more TypeScript-friendly?

I've found it fine to use this package with ts-node, so might start with simply supporting ts template generation.

colinwirt avatar Mar 05 '21 05:03 colinwirt

I have to admit I haven't had much time recently to work on this project, as I'm not actively using Contentful anymore. Also it does not help that I don't have much experience with TS, and not much opportunities to experiment with it.

I put my comments on @trptcolin 's PR with my thoughts on the approach I'd like to implement, and I think (IMHO), that it should be straightforward to do that way

What does not help is that seems like the integration tests are broken, and I have to spend some time fixing them before anything else is done in the project.

deluan avatar Mar 05 '21 20:03 deluan

Thanks @deluan :)

What does not help is that seems like the integration tests are broken, and I have to spend some time fixing them before anything else is done in the project.

I was thinking that that might be something holding up approvals. Is that something I can help with fixing?

colinwirt avatar Mar 08 '21 06:03 colinwirt

Yes it is, and help would be very welcome, thanks for offering it! Send me an email if you decide to tackle it and need some help on setting up the environment to run integration tests locally.

deluan avatar Mar 08 '21 15:03 deluan

We can get acceptable typescript support with only a couple of changes:

  1. Add // @ts-check to the top of the migration template
  2. Add the following comment block above both exports.up and exports.down:
/**
 * @param {import("contentful-migration").default} migration
 */

This is reasonable non-invasive, and still allows us to use Typescript checking without waiting on contentful to land changes to contentful-migrations. I'm happy to prepare a PR for this change if you like.

bgschiller avatar Mar 28 '22 17:03 bgschiller