rewrite to handle typescript

[lordnox]

Here is my take on converting your codebase into typescript.

I have not changed any of your API, it's working quite nicely. The v8-types is a copy & paste from here.

If you have any questions, I am happy to answer.

In regards to the build process, I am using rollup (see rollup.config.js): npm run build to create a /dist directory with all files needed. npm run watch to rebuild after any change, for development npm run test to run tests, everything is configured to use *.spec.ts-files npm run test:watch to run the tests while working on the code npm run test:coverage to run tests on any CI system to get a total coverage (to show in PRs for example)

I did not write any tests though, there were none. If you start with this I am happy to help as well.

[lordnox]

Oh, I changes the export type from module.exports = ... to export { Todoist, v8 }

This, and the changes in the API itself, will have to be addressed in the README. I did not touch that file yet.

[romgrk]

Looks good overall :)

A few comments:

• Can we export default v8? We're still pre-1.0.0 but I'd rather avoid breaking changes if possible
• Besides that, are there other API changes that need to be documented in the README?
• The node_add command handles both Item Notes and Project Notes, but takes either { item_id: xx, ... } or { project_id: xx, ....}
• Is it possible to adjust the tsconfig to use *.test.* for tests, rather than *.spec.*? I find it slightly more clear

About the tests, yeah it would be good to write some, I could try to make some time to start with that once this PR is merged.

One question for you, how do you find the API currently? I've been thinking about modifying it to handle offline transactions/multiple commands, but that would require a lot of bookkeeping and I'm not sure how useful it would be.

[lordnox]

1. Can we export default v8? We're still pre-1.0.0 but I'd rather avoid breaking changes if possible Yes that is possible, but it will be available as 'default'. You cannot mix named exports and default exports in typescript. I'd recommend using named exports here as it is more compatible with everything. import { Todoist } from 'todoist'; and const { Todoist } = require('todoist');. This might save 2 characters.

2. Besides that, are there other API changes that need to be documented in the README? I don't think I changed anything in the API. I removed all the (options) => command(…, options) calls, but the logic stayed the same. I cannot guarantee that nothing changed... tests would have been nice.

3. The node_add command handles both Item Notes and Project Notes, but takes either { item_id: xx, ... } or { project_id: xx, ....} Yes, I update the PR, there is another way to do this though:

   const notes = {
   get: () => state.notes,
   add: createCommand<Types.NoteAdd | Types.ProjectNoteAdd>('note', 'add'),
   update: createCommand<Types.NoteUpdate | Types.ProjectNoteUpdate>('note', 'update'),
   delete: createCommand<Types.NoteDelete | Types.ProjectNoteDelete>('note', 'delete'),
   }

   This would allow the user to run api.notes.add({ /* some project note */} OR { /* some not */}). In the PR I decided to create the projectNotes endpoint, using the same commands as notes.

4. Is it possible to adjust the tsconfig to use .test. for tests, rather than .spec.? I find it slightly more clear Sure, I will update the PR

[lordnox]

One question for you, how do you find the API currently? I've been thinking about modifying it to handle offline transactions/multiple commands, but that would require a lot of bookkeeping and I'm not sure how useful it would be.

Hm... good question.

I do like this API, it resembles the todoist API with the sync functionality quite well. I'd probably added an autosync feature, or a queue. But that is too early I would say.

For a roadmap I would start with the queue, as the transaction feature would be a given with it.

transaction:

• start transaction (pause queue)
• commit transaction (unpause queue)

canceled transaction:

• start transaction (pause queue)
• cancel transaction (reset queue)

But I would say a good test suite, implementation of all endpoints and adding the correct return values & types to the commands should get priority.

[romgrk]

Ok, LGTM, minor adjustments before merging:

• The note_update and note_delete commands don't take a project_id for project notes, they take only the note's id so they're identical to note_update and note_delete for item notes.
• Can you adjust the README's usage section so that copy-pasting the code there still works? Just adding a .v8 after the require call will do.

Just to mention, currently the .projectNotes endpoint won't work as expected because executeCommand takes the type (eg createCommand('note', 'add'): type is 'note') to do some magic and return the operated on record. But this can be dealt with later.

[lordnox]

The note_update and note_delete commands don't take a project_id for project notes, they take only the note's id so they're identical to note_update and note_delete for item notes.

I am not sure what you mean by this. I am c&p everything type-related from here the API documentation. If you find problems please tell me, and write todoist an email 😉

I fixed the projectNotes.get endpoint, it takes its type from the interface State. And yesterday evening I just missed that.

In regards to the return type of executeCommand, there will have to be an update to the executeCommand calls that define the ReturnType. Currently it is NodeType | undefined. It is possible to take the type of the command name executeCommand('notes', 'add') would return a Note, however I think it is better and more explicit to tell the system what is returned executeCommand<Note>('notes', 'add'). The default return type would be undefined and can be overwritten like above.

[romgrk]

Do you want to make the change to executeCommand in this PR or in subsequent work?

And here is what I mean for the note_update and note_delete commands, currently the types in this PR don't seem to match the documentation for project notes. Do you see what I see?

In other words, note_add takes either item_id or project_id for item or project notes, but once the note is created the commands that operate on it (note_update & note_delete) are identical for items & projects, type-wise.

[lordnox]

Oh.. That is a copy&paste error on my part. I'm updating the types of ProjectNoteUpdate and ProjectNoteDelete. You are correct that the Todoist API node_add command would take either item_id or project_id, the API is currently enforcing api.note.add({ item_id }) and api.projectNote.add({ project_id }). If you try the other way around typescript would prevent that. Compiled to JS it still works, there is no enforcement of this in the compiled code.

api.note.add can take the value uids_to_notify while api.projectNote.add cannot

[romgrk]

Ok, one last modification, can you add an .npmignore with only node_modules? Otherwise npm publish won't publish dist.

I've added you as a collaborator to this repo, if it's ready after that modification you should be able to merge yourself.

[lordnox]

.npmignore added