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

refactor: migrate library to TypeScript #40

Merged
merged 8 commits into from
Sep 10, 2021
Merged

refactor: migrate library to TypeScript #40

merged 8 commits into from
Sep 10, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Sep 4, 2021

So here it is—a library completely in TypeScript! This will be the first step towards v2.

What I did:

  • Made src/ the TS root and dist/ the output;
  • Implemented ReadingTimeStream as a class;
  • Configured ESLint to be TS-compatible.

How is publishing different?

The dist and src folders should both be published, and since the former is now gitignore-d, it won't be available until you run npm run build. I've configured the prepublishOnly script to run build every time when running npm publish.

Expected output:

npm notice 2.1kB README.md            
npm notice 142B  lib/index.d.ts       
npm notice 258B  lib/index.js         
npm notice 262B  lib/reading-time.d.ts
npm notice 3.3kB lib/reading-time.js  
npm notice 553B  lib/stream.d.ts      
npm notice 2.1kB lib/stream.js        
npm notice 1.1kB license              
npm notice 957B  package.json         
npm notice 139B  src/index.ts         
npm notice 3.1kB src/reading-time.ts  
npm notice 1.0kB src/stream.ts        
npm notice 694B  src/types.d.ts       
npm notice 122B  tsconfig.json

Note: after merging, please publish it as 2.0.0-alpha.0, because we're only halfway through the v2 milestone, but I'd still like to have a published version to test the current changes in other projects.

Breaking Changes

  • The IOptions and IReadTimeResults no longer exist, in favor of the Options and ReadTimeResults types;
  • The ReadingTimeStream is no longer instantiable without explicitly using new;
    const {ReadingTimeStream} = require('reading-time');
    
    // Instead of:
    fs.createReadStream('foo')
      .pipe(ReadingTimeStream)
    
    // Please use:
    const analyzer = new ReadingTimeStream()
    fs.createReadStream('foo')
      .pipe(analyzer)
    • Why? This makes it clearer that ReadingTimeStream is a class rather than a function. In year 2021, we should embrace strict type enforcement and stay away from the wackiness of weak typing.

@Josh-Cena Josh-Cena mentioned this pull request Sep 4, 2021
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena mentioned this pull request Sep 10, 2021
Copy link
Owner

@ngryman ngryman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the great work 🙌

This LGTM for the first step towards v2. One question on the top of my mind: do you expect to convert tests to Typescript as well in a follow-up PR?

Also at this point, I think it would make sense to add you as a collaborator to this project. I'm going to do that right now 🚀

Comment on lines +1 to +9
import readingTime from './reading-time'
import ReadingTimeStream from './stream'

// Wacky way to support const readingTime = require('reading-time') :(
// Basically we can't use ES import/export anymore because re-assigning module.exports
// decouples it from the exports object, which TS export compiles to
module.exports = readingTime
module.exports.default = readingTime
module.exports.ReadingTimeStream = ReadingTimeStream
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work:

Suggested change
import readingTime from './reading-time'
import ReadingTimeStream from './stream'
// Wacky way to support const readingTime = require('reading-time') :(
// Basically we can't use ES import/export anymore because re-assigning module.exports
// decouples it from the exports object, which TS export compiles to
module.exports = readingTime
module.exports.default = readingTime
module.exports.ReadingTimeStream = ReadingTimeStream
import readingTimeFn from "./reading-time";
export default readingTimeFn;
export const readingTime = readingTimeFn;
export { default as ReadingTimeStream } from "./stream";

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with export default readingTimeFn; is it's transpiled to module.exports.default = readingTimeFn. I need to support the require() without .default because that seems less straightforward.

I've asked the TS community if they have anything good and let's wait for a couple of days to see 👀

@Josh-Cena
Copy link
Collaborator Author

do you expect to convert tests to Typescript as well in a follow-up PR?

Yes, I do😁 Not so experienced with Mocha so I'll do it when I'm migrating to Jest

Thanks for inviting! Feel honored

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Sep 10, 2021

@ngryman So let's merge this for now, and you can make another 2.0.0-alpha.0 release so I can test in my repo 😏 We can figure out about the right export method later

@Josh-Cena Josh-Cena merged commit 8f95bbb into ngryman:master Sep 10, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants