Migrate to TypeScript

[Khartir]

So @phryneas and I had some slacktime ...

[phryneas]

This is not really meant to be switched to right now, because the types are mostly the same as the hand-written types from before.

This is just a (mostly) 1:1 conversion of the JS code. It can be used as a starting point to experiment with the types to determine what could be changed with the APIs.

[phryneas]

Also, some things with the build might still need improvements - our next slacktime is in two weeks and we are looking forward to putting some more work in there.
So getting some feedback from you would be great - and if you want to do some experimentation too, all the better :)

[codecov]

Codecov Report

Merging #105 into next will decrease coverage by 1.13%.
The diff coverage is 97.78%.

@@            Coverage Diff             @@
##             next     #105      +/-   ##
==========================================
- Coverage   98.93%   97.79%   -1.14%     
==========================================
  Files           8        8              
  Lines         375      408      +33     
  Branches      140      143       +3     
==========================================
+ Hits          371      399      +28     
- Misses          4        9       +5

Impacted Files	Coverage Δ
packages/react-async/src/propTypes.ts	100% <ø> (ø)
packages/react-async/src/Async.tsx	100% <100%> (ø)
packages/react-async/src/status.ts	100% <100%> (ø)
packages/react-async/src/reducer.ts	100% <100%> (ø)
packages/react-async/src/useAsync.tsx	99.15% <100%> (ø)
packages/react-async/src/helpers.tsx	80.76% <80.76%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91a7d88...3381fbe. Read the comment docs.

[ghengeveld]

Awesome work guys! Thanks for that. Big plans are emerging, and TS is part of that.

[codecov-io]

Codecov Report

Merging #105 into next will decrease coverage by 0.14%.
The diff coverage is 98.8%.

@@            Coverage Diff             @@
##             next     #105      +/-   ##
==========================================
- Coverage   98.72%   98.58%   -0.15%     
==========================================
  Files           8        8              
  Lines         393      424      +31     
  Branches      143      148       +5     
==========================================
+ Hits          388      418      +30     
- Misses          5        6       +1

Impacted Files	Coverage Δ
packages/react-async/src/propTypes.ts	100% <ø> (ø)
packages/react-async/src/Async.tsx	100% <100%> (ø)
packages/react-async/src/helpers.tsx	100% <100%> (ø)
packages/react-async-devtools/src/index.js	94.73% <100%> (ø)	⬆️
packages/react-async/src/status.ts	100% <100%> (ø)
packages/react-async/src/reducer.ts	94.11% <94.11%> (ø)
packages/react-async/src/useAsync.tsx	99.22% <99.22%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f1c1b1...9a50543. Read the comment docs.

[ghengeveld]

@slorber could you take a look?

[phryneas]

Oh, for better reviewing you might probably want to look at the diff excluding the "file rename" commit, as that's much cleaner:

Khartir/react-async@d068194...switch-to-ts

[Avi98]

hi @phryneas and @ghengeveld I'm a big fan of react-async & typescript and would react really love to contribute to this PR

[phryneas]

@Avi98: A review would be highly welcome!

Also, I believe there are still some TODOs in that code, you could tackle those if you wanted :)
Aside from that: the examples are not compiling yet, so that would be a point to look. And the build process still needs an additional step that compiles all .d.ts into one output file. (users consuming this via babel-preset-typescript instead of ts-loader will have problems otherwise, as babel does not work very good with re-reports of types)

Those are all open points I can think of right now :)

[ghengeveld]

Also make sure it works with Pika: https://www.npmjs.com/package/@pika/plugin-ts-standard-pkg

[phryneas]

Also make sure it works with Pika: https://www.npmjs.com/package/@pika/plugin-ts-standard-pkg

Yup. Although I believe that this would need a PR against pika or a new pika plugin. Current functionality does not cover the declaration-merging. For some pointers on how to implement something like that, there is some discussion over at tsdx as they are facing similar problems: jaredpalmer/tsdx#80

[FredKSchott]

Hey y'all, maintainer of Pika here. Happy to help contribute however I can if something is lacking on the @pika/pack side of things

[phryneas]

Hey y'all, maintainer of Pika here. Happy to help contribute however I can if something is lacking on the @pika/pack side of things

@FredKSchott very cool! Could you take a look at that "bundle all .d.ts into one .d.ts" thing? The issue I linked above contains a few libraries that might be a solution. I've tried @microsoft/api-extractor(as I wasn't using rollup) in another project and it wasn't working very well for me. But maybe one of those rollup plugins does - and pika is already using rollup as I see, so those wouldn't even introduce too many extra dependencies.

[FredKSchott]

Sure thing, we actually already have a "bundle types" build plugin that should do what you need: https://github.com/pikapkg/builders/tree/master/packages/plugin-bundle-types

Try that out, and then if anything's missing or broken let me know and I'd be happy to take a look

[phryneas]

Thanks!

This seems to do exactly what we need - but there's a problem in the generated file. I've opened an issue!

[Avi98]

users consuming this via babel-preset-typescript instead of ts-loader will have problems otherwise, as babel does not work very good with re-reports of types

@phryneas are you suggesting to add webpack config and configure ts-loader

[phryneas]

No, this is about people using such a setup that would be unable to use this library if we shipped multiple definition files with re-exports. I would not want to introduce webpack up the build process of this package 😁 -- Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[Avi98]

@phryneas I think you can run examples by running following commands

yarn && yarn build:packages
yarn bootstrap
yarn start:examples

also if you can join discord channel so that I can get more inputs from you

[phryneas]

@avinash I'm on a conference right now, will check it next week when I'm back home 🙂

…

-- Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[phryneas]

And we're all green 🎉

[ghengeveld]

@FredKSchott could you shed some light on the recommended package setup?

Right now our releases contain the following:

  "esnext": "dist-src/index.js",
  "types": "dist-types/index.d.ts",
  "main": "dist-node/index.js",
  "module": "dist-web/index.js"

This PR, using @pika/plugin-ts-standard-pkg would change it to:

  "source": "dist-src/index.js",
  "types": "dist-types/index.d.ts",
  "main": "dist-node/index.js",
  "module": "dist-web/index.js"

We would like to offer support for IE11, so people can safely use the package directly from a CDN. Probably adding @pika/plugin-build-umd would do the trick. I think that would give us:

  "source": "dist-src/index.js",
  "types": "dist-types/index.d.ts",
  "main": "dist-node/index.js",
  "umd:main": "dist-umd/index.js",
  "module": "dist-web/index.js"

Is this a sensible setup? Do we really need all of them? What about the esnext entry point? I'm not familiar enough with the various build systems to tell. We want to target relatively modern browsers as well as Node.js.

Also: what do we need to have the Pika CDN serve it properly so it can be used in a script tag?

[ghengeveld]

Do we still need @pika/plugin-standard-pkg now that we're using @pika/plugin-ts-standard-pkg?

[phryneas]

Do we still need @pika/plugin-standard-pkg now that we're using @pika/plugin-ts-standard-pkg?

Good point, I'll remove that dependency.

PS: ah, no, still need that for the devtools package.

[FredKSchott]

@ghengeveld yup, from a quick glance that looks good. Here a quick summary of each:

  "types":  Types for TypeScript users
  "main": Entrypoint for Node.js
  "module": Entrypoint for Modern browsers, bundlers
  "umd:main": Entrypoint for legacy browsers

One suggestion (but not required) is that you could add CDN-specific entrypoints for some of the older JS CDNs. UNPKG (and JSDelivr, iirc) will both point to "main" instead of "module" by default for legacy reasons, so it can help to tell them to point to your web build explicitly:

- ["@pika/plugin-build-web"],
+ ["@pika/plugin-build-web", { "entrypoint: ["module", "unpkg", "jsdeliver"] }],

"source" is just like "esnext", either can be used to point to the most modern version of your source code. Neither are really used by anyone, but can be useful for the odd tool/bundling use-case.

[FredKSchott]

Once you publish, you can check out https://www.pika.dev/npm/react-async/cdn to see the result/status of the CDN build.

I'll plug our Pika Package CI GitHub integration as well, which tests that no PR breaks your package from being properly bundled by the consumer (via Rollup/Webpack) and then gives you a preview install URL if you want to install and try any PR locally. If it passes that check, it's more or less guaranteed to build on our CDN as well.

[ghengeveld]

Thanks @FredKSchott that will work. I think this is what we should have:

  "@pika/pack": {
    "pipeline": [
      ["@pika/plugin-ts-standard-pkg"],
      ["@pika/plugin-build-node"],
      ["@pika/plugin-build-web", { "entrypoint: ["module", "unpkg", "jsdelivr"] }],
      ["@pika/plugin-build-umd"],
      ["@pika/plugin-bundle-types"]
    ]
  }

Note that it should be "jsdelivr" not "jsdeliver".

[phryneas]

Okay, now that part should be okay, too.

[ghengeveld]

I'm going to merge this, but I'd like some background on the resolutions:fix-react script? @Khartir or @phryneas can you explain what we need it for? I don't really feel comfortable having to maintain that script if I don't know the story behind it.

[ghengeveld]

🚀

[phryneas]

I'm going to merge this, but I'd like some background on the resolutions:fix-react script? @Khartir or @phryneas can you explain what we need it for? I don't really feel comfortable having to maintain that script if I don't know the story behind it.

That is due to this coment: #105 (comment)
Essentially, switching between react versions to run the tests in different versions doesn't go 100% smoothly - somehow, a mismatching react and react-dom appear during switching versions.
That script edits the package.json to have a resolutions.react and resolutions.react-dom that are exactly equal to devDependencies.react and re-runs yarn install. That way, those are guaranteed to be 100% consistent in the CI.