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

Transition to native ES Modules + TypeScript declaration files #2128

Open
8 of 10 tasks
Tracked by #2218 ...
gr2m opened this issue Jul 9, 2021 · 3 comments
Open
8 of 10 tasks
Tracked by #2218 ...

Transition to native ES Modules + TypeScript declaration files #2128

gr2m opened this issue Jul 9, 2021 · 3 comments
Assignees
Labels
project typescript Relevant to TypeScript users only

Comments

@gr2m
Copy link
Contributor

gr2m commented Jul 9, 2021

Depends on #2127, because migrating to ES Modules will mean that we will have two versions we need to support for the foreseeable time (the latest which will be ESM and a long-term support version implemented as commonjs)

Problems

  • We currently don't have control over the resulting declaration files. They are hard to read and debug. And with growing complexity of Octokit's decomposable and pluggable architecture it's increasingly hard to do so with TypeScript source code
  • Users who run into TypeScript problems have a hard time to debug their problem and to submit fixes, because the *.d.ts files are not checked into the source code on GitHub. They are generated and published to npm only. Example: https://unpkg.com/browse/@octokit/[email protected]/dist-types. The relation between the source code (in this case: https://github.com/octokit/core.js/tree/v3.5.1/src) and the definition file is not always straight forward.
  • Different versions of the TypeScript compiler (tsc) and different compiler options result in different declaration files.
  • When writing the entire source code in TypeScript, types are checked throughout the entire code. That can be helpful at times, but more often than not it results in unneeded code complexity and build errors that slow down the development and maintenance of the Octokit source code. I estimate that the introduction of TypeScript slows down code develepoment by a factor of 4x-5x. I think separating the source code from the types will bring it down to 2x-3x, as only the types for the plublic APIs need to be defined and tested.

Solution

Rewrite the TypeScript source files into native JavaScript and add the .d.ts declaration files into the source code. Besides testing the code, add tests for types.

Benefits

  • Removes all build steps. We currently use @pika/pack which is now unmaintained. The source code in the GitHub repositories is 1:1 what is used in production. The runtime code will be easier to read, debug, and contribute to.
  • Source code is production code. Easier to debug and contribute to.

Steps

  • Create a new octokit/octokit-next.js repository as a native ES Module with separate TypeScript Declaration files. There should be no build step necessary.

  • Once the setup is working for octokit/octokit-next.js, start the transition to ES Modules and .d.ts files with the most low-level dependencies.

    Separate the Node usage into Node CommonJS (legacy) and Node ES Modules (12+), e.g. in https://github.com/octokit/endpoint.js#usage. Use npm install @octokit/endpoint@commonjs (etc) for the legacy usage. Mention the long-term support version that is publishing to the @commonjs release channel (dist-tag) in the README and link to it.

    • @octokit/endpoint
    • @octokit/request-error
    • @octokit/request
  • Make sure that all outside dependencies such as deprecation and before-after-hook are either built as ES Modules themselves or are fully compatible. Of particular interest is node-fetch. See v3 Roadmap node-fetch/node-fetch#668. See Remove node-fetch by default octokit-next.js#57

  • For @octokit/request, test the conditional exports for Node (which requires the node-fetch polyfill) and Browsers/Deno which do not. Make sure that the published packages work with browsers and deno via https://www.skypack.dev/. see Remove node-fetch by default octokit-next.js#57

  • Once the new setup and release automation is working for the packages listed above, propagate it to all other @octokit/* packages

  • Setup automation which helps backport new bug fixes and features to the @commonjs long-term support version. Expand the current automation around OpenAPI spec updates, webhooks updates and app permission updates to also send pull requests against the @commonjs long-term support version branches.

@gr2m
Copy link
Contributor Author

gr2m commented Jul 19, 2021

Of particular interest is node-fetch. See node-fetch/node-fetch#668

Starting with v3.0.0-beta.10 node-fetch is a native ES Module. But turns out that fetch-mock fails when combined with that node-fetch version

@gr2m gr2m self-assigned this Aug 16, 2021
@gr2m gr2m pinned this issue Aug 16, 2021
@jimmywarting
Copy link

jimmywarting commented Nov 11, 2021

Glad that i'm not the only person that feels that Typescript are only in the way. I also think the compiling is wasteful, and you can't always use the latest features, IMO i don't think typescripts fits well into a loosed typed language. It's possible to have type safety with jsdoc and vanilla javascript without it. I think the hole extension less and the new introduction of mts and cts to imports for typescript, Deno and nodejs was their downfall. Remote path resolving is a nightmare to get it right.

Deno stop using Typescript for a couple of reasons i can only agree upon

I would like to help out in anyway i can to migrate to vanilla javascript and switching over to ESM, just tell me what i can do

@gr2m
Copy link
Contributor Author

gr2m commented Nov 11, 2021

Thanks Jimmy! The work is on hold right now, but I'll let you know once we get back to it.

It's not a simple transpilation of TS -> JS, it's an entire rewrite in many ways, which among other things enables the separation of REST API types and code, making Octokit easier to use with GitHub Enterprise Server and other special environments. The WIP is here: https://github.com/octokit/octokit-next.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project typescript Relevant to TypeScript users only
Projects
None yet
Development

No branches or pull requests

2 participants