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

Feature request: Include TypeScript .d.ts file with the npm package #128

Closed
ristomatti opened this issue Aug 9, 2018 · 16 comments
Closed

Comments

@ristomatti
Copy link

Expected Behavior

As the project is written in TypeScript, I would expect it to also allow using the tool with it's typings and to benefit from the defined types when writing migration scripts.

Actual Behavior

The npm package includes only the transpiled code so no types are available.

Possible Solution

Create a separate .d.ts file for the API calls used in the documentation and include it in the npm package.

@Khaledgarbaya
Copy link
Contributor

It will be possible when #68 is merged.
the only thing that stopping the merge is the merge conflict

@ristomatti
Copy link
Author

I tried rebasing it myself as the only conflict was the README.md but I could not get it to work easily. The example imports are also referring to contentful-migration-cli, has this repo been renamed prehaps? I would suggest to rebase it yourself while having actual understanding of the inner workings of the code base. A lot of work done there already.

It seems to me the developer is not planning to rebase the PR as the PR seems to have evolved into a forked tool: https://github.com/watermarkchurch/migration-cli

@Khaledgarbaya
Copy link
Contributor

Hey @ristomatti,
I pushed some fixed to the PR, can check now.
If all good I can go ahead and merge

Best,
Khaled

@ristomatti
Copy link
Author

@Khaledgarbaya Thanks! I'll give it a shot when I get the chance. I'm currently using Contentful within a customer project so probing around with new tools needs to be when things aren't that busy.

@ristomatti
Copy link
Author

I believe I got this working on a custom project directory using npm link. I'm not sure if the ts-node example would be a good idea to include in the documentation though. It seems kind of hacky especially considering the recommended way to run migrations on command command line would be using contentful-cli right?

Maybe a more future proof documentation would mention the TypeScript migration scripts would first need to be transpiled to JavaScript?

@Khaledgarbaya
Copy link
Contributor

Hey @ristomatti That sounds promising can you share the project somewhere and I will try to have an example based on that in the docs

@ristomatti
Copy link
Author

Sorry it's not in a state to be shared, I just experimented with using a tsconfig.json, type annotated JavaScript files etc. I decided to leave the experimentation for now until the typings are part of the npm package. I'm designing/creating a migration script development workflow and deployment that will be used by multiple developers already and I doubt the PR will be merged before I'm done. It's not an option in this case to use a forked branch.

The most important goal is to find the Contentful tool/library for the purpose that would most likely be supported in the future. contentful-cli is the tool recommended in the documentation but it is in beta and has some bugs that make me hesitant to include here. But as it uses contentful-migration in the background, I'm currently leaning towards creating a simplified CLI deployment tool that uses contentful-migration as a library to run the scripts (written in JS for now).

@ristomatti
Copy link
Author

@Khaledgarbaya I ended up doing some more testing on this today after all. What I did was just take the index.d.ts file, put it to ./typings/contentful-migration folder and set ./typings as one of the tsconfig.json typeRoots. As hacky as the example seemed it did work. It took a while to figure out how it works but eventually was able to run a TypeScript migration from another TypeScript file based on one of the JavaScript examples for using contentul-migration as a library.

The typings seem to not cover everything though. To get no errors from the script I had exported from our production system I had to add to ContentType:

export interface ContentType {
  /** Change field editor interface. */
  changeEditorInterface(fieldId: string, widgetId: string, settings: Object): void
}

and change IValidation property in to also accept number[]:

export interface IValidation {
  /** Takes an array of values and validates that the field value is in this array. */
  in?: string[] | number[],
}

@Khaledgarbaya
Copy link
Contributor

Hey @ristomatti,
That looks promising, I will allocate time for the branch tomorrow and do some tests with your changes and try to get into master.

Best,
Khaled

@joshmedeski
Copy link

Where are we at with this? I'm looking to start using this tool and Typescript would be great!

@ristomatti
Copy link
Author

I've been using the typings file in a project using ts-node and It works great! The repo is unfortunately private so I cannot link to it. I have though done some more additions/improvements on the typings file after testing it with a migration script dump from an existing system.

I can create a pull request once this is merged.

@Khaledgarbaya
Copy link
Contributor

Hey @ristomatti @joshmedeski,
We'll tackle this hopefully next week.

@joshmedeski
Copy link

Ok, so I followed the updated README:

node_modules/.bin/ts-node node_modules/.bin/contentful-migration -s $CONTENTFUL_SPACE_ID -a $CONTENTFUL_MANAGEMENT_TOKEN my_migration.ts

But I'm getting the base content when you type contentful-migrate on it's own:

Parses and runs a migration script on a Contentful space.
...

And the following error at the bottom:

q is not defined

It seems like the contentful-migration script isn't taking the arguments.

@ristomatti
Copy link
Author

@joshmedeski I'm not at the computer right now but try adding -- before the -s parameter. We're using this with the "use as a library" method. I have created a .ts file which imports the cli module similarly as in the JavaScript example. The parameter handling is done by the script though.

I'm pretty sure though I did the initial test with the command from the new documentation though...

@Khaledgarbaya I noticed the changes being applied through commits today. I'll try to find time tomorrow to do a PR on the improvements on the typings file.

@Khaledgarbaya
Copy link
Contributor

Hey @ristomatti,
Thanks a lot, I will close this issue for now since the first implementation is released.

@ristomatti
Copy link
Author

@Khaledgarbaya it seems all but one of the changes I've done have been added there meanwhile. I don't bother making a PR from that.

The only change missing was adding | number[] to IValidation.in. I added it as otherwise migration file generated with contentful space generate would not have been valid.

export interface IValidation {
  in?: string[] | number[],

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

No branches or pull requests

3 participants