search

webrecorder / warcio.js

JS Streaming WARC IO optimized for Browser and Node
MIT License
30 stars 6 forks source link

Port to TypeScript #44

Closed chelkyl closed 1 year ago

chelkyl commented 1 year ago

Changes include:

We attempted to preserve code structure and compatibility as much as possible while adding type safety.

Thank you for creating this library!

ikreymer commented 1 year ago

@chelkyl @jlarmstrongiv wow, thank you for all this work! It was on our roadmap to convert to TS eventually, but hadn't planned on it yet. We'll try to review it quickly. Some quick questions to start:

switched from yarn to npm switched from rollup to tsup switched from ava+c8+codecov to jest

Just wondering about any particular reasons for these changes, especially for testing one?

jlarmstrongiv commented 1 year ago

@ikreymer great news! We really like this library and we’re glad we can contribute to it 😄 thank you for helping review these changes. Please let us know if you have more questions:

switched from yarn to npm

Several years ago, yarn had substantial feature and performance improvements over npm. However, many of these changes have been added back into npm, so I no longer feel the need to add an extra step with yarn. I also haven’t closely followed yarn after they released v2/berry and the new pnpMode, with several companies staying with yarn v1/classic or switching back to npm. It should be an easy change to revert back to yarn if you prefer it.

switched from rollup to tsup

Tsup is a no config bundler based on esbuild and rollup, with typescript support out of the box. Plus, it supports both ESModules and CommonJS. I found it to be the easiest way to switch to migrate to typescript for warcio.js and other projects.

switched from ava+c8+codecov to jest

By switching from ava+c8+codecov to jest, we were able to replace three tools with one more popular tool that does it all. I have also found jest to be easier to setup for TypeScript, multiple environments (nodejs and browser), and adding polyfills.

ikreymer commented 1 year ago

Made a couple of comments, mostly adding additional exports. Also pushed (1.6.1) release which was released but forgot to push to GH (sorry!) that just disables the min node version.

With these, I was able to get it to build in wabac.js, which is one of the main libraries using this.

The main issue I'm running in now, is with the test suite in wabac.js is with the pako types, which in 1.0.x do not include the proper exports. This repros with just a node script that has:

import { WARCParser } from 'warcio';

which fails because pako doesn't export Inflate in 1.0.x. What is the correct solution for this when used in JS modules? (I think there's other changes that prevented moving to pako 2.x for now). Ideally, the new warcio 2.0 could just be used in the same way in an ESM modules?

ikreymer commented 1 year ago

Added a commit that I think fixes the issues so far! Just using import pako from "pako" to resolve the issues in upstream libraries, and also added a utils only export and removed cli commands from main export.

jlarmstrongiv commented 1 year ago

@ikreymer thank you so much for reviewing and fixing 😄 glad it’s looking good!

ikreymer commented 1 year ago

@ikreymer thank you so much for reviewing and fixing 😄 glad it’s looking good!

Yep, Thanks for starting this! Just a couple of things left, for example:

  • asyncIterReader.readSize now returns an array of the read size and the read chunks instead of sometimes the read size and sometimes the read chunks for easier typing

Hm, I think this is less than desirable, but see why it was needed to make it work. Will see if can refactor it to just return the chunk read, and maybe do skipping differently..

jlarmstrongiv commented 1 year ago

Thank you so much for your help reviewing, fixing, and merging @ikreymer 🎉 glad to see this update land