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(typescript): refactored code to use typescript #30

Closed

Conversation

dvelasquez
Copy link

This PR refactors the code to use TypeScript instead of VanillaJS.

Changes

  • Updated eslint to handle typescript code
  • Refactored source files to typescript
  • Refactored test files to typescript
  • Fixed some issues with the tests
  • Updated package.json to expose CJS and ES Modules
  • Created Type Definition for the PageXRay module

@mrchrisadams
Copy link
Member

HI @dvelasquez, I've had a chance to look over this, and I really like it - thanks for taking the time to work on it.

I'd like to merge this in with fresh eyes though, so I'm going to check tomorrow to see what changes might be needed in code that consumes the earlier released version of co2.js, then then, and update the changelog and README, to prep for a new release.

Much gracias 👍

@dvelasquez
Copy link
Author

Hey! It would be really interesting to try this branch with a project that uses this as a dependency, to check if everything is ok

@mrchrisadams
Copy link
Member

Indeed!

Sitespeed would be the big one, as it's in production in a few high profile places. Here's an example of their public dashboards:

Screenshot 2020-10-02 at 18 22 22

And below is the direct link to the chart:

https://dashboard.sitespeed.io/d/000000060/leaderboard?orgId=1&from=1603728284785&to=1604333084785&var-namespace=sitespeed_io&var-path=alexaDesktop&var-domains=All&var-pages=All&var-browser=chrome&var-connectivity=cable&var-function=median&viewPanel=79

I had planned to:

  • a checkout of sitespeed
  • update the dependencies to point to this branch
  • run the tests and see if we get the numbers

This here where it's in use in the sitespeed codebase:

https://github.com/sitespeedio/sitespeed.io/blob/main/lib/plugins/sustainable/index.js#L8-L13

And the tests looked like this, but they've since been moved into a separate repo:

https://github.com/sitespeedio/sitespeed.io/pull/2868/files

Copy link
Contributor

@drydenwilliams drydenwilliams left a comment

Choose a reason for hiding this comment

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

Looks pretty great to me 👍

note: haven't run it locally

@mrchrisadams
Copy link
Member

hi @dvelasquez thanks so much for this PR. I'm going to close this, as there have been a bunch of changes since this PR was initially created - both in this project, in typescript and in javascript too.

Porting this to typescript isn't on the short term roadmap for now, but we're collecting any discussion in the issue below, about whether to add it later:

#77

@dvelasquez
Copy link
Author

No problem, let me know if you want me to tackle this again in the future.

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.

3 participants