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

Migrate to Typescript #31

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

JumpLink
Copy link

@JumpLink JumpLink commented Jan 5, 2022

  • Migrate to Typescript
  • Add builds for
    • types - Optional for those who use Typescript
    • browser - The Webpack bundle
    • ESM - Optional for those who want to use gamecontroller.js as a module in there own bundler
    • CommonJS - Useful if you want to use gamecontroller.js in node e.g. with JSDOM, Jest or Webpack (Is now also used internally for the Jest tests)
  • Add Github action to run the Jest tests on any commit, see example
  • Update tests and examples so that everything continues to work
  • Add serve script to run the examples locally
  • Add a webpack.config.js to transpile Typescript using Babel for the browser bundle output

@alvaromontoro
Copy link
Owner

Hi @JumpLink, thank you for creating this PR. Migrating to Typescript was not in the roadmap in the short/mid-term. I will need to review these changes thoroughly (it is a big change) and that will take time.

@JumpLink
Copy link
Author

JumpLink commented Jan 22, 2022

@alvaromontoro yes I understand. I thought it would make more sense (and not really more effort) to port the project itself to Typescript instead of writing separate types for @types.

Thanks to the typescript precompiler, it was also easy to build the package for ESM and CJS, so I did that as well.

To stay backwards compatible I left the webpack build in, so this bundle can still be used if you don't want to use npm or any bundler in your own project.

So the result is:

  • Project migrated to Typescript
  • Packages supports ESM, CJS and a minified bundle direct for the browser
  • And the other things I mentioned above that came up e.g. testing the tests etc

Possibly the types can be further improved and thanks to Babble the APIs for old browsers can also be omitted in the future. For this I also had to use the any type in TypeScript, since the old APIs are no longer available in the DOM TS types. I left them in so as not to damage anything

I would be happy if the PR would be merged and I already have another idea for a future separate PR.

@alvaromontoro
Copy link
Owner

Sorry, it's taking me so long. At the moment, I don't have the bandwidth to take care of such a big review :(

@JumpLink
Copy link
Author

JumpLink commented Mar 8, 2022

@alvaromontoro Yes I understand that, don't stress yourself. Since the codebase is manageable I would just look at and test the result instead of review the PR's diff directly.

At the moment I'm busy with other projects myself but in the future I have some more features in mind that I would like to submit. For example, Firefox seems to map some controllers differently than Chrome on Linux. I haven't tested if this is also the case on other operating systems, but we could add the possibility to make the mapping of the buttons configurable ( maybe depending on the browser /os) and in this way align the controller mapping of Firefox.

Do you have a kind of roudmap so that I can assess which features you have already planned for yourself in the future?

However, I would then want to submit such a PR based on this PR. Therefore, I would be happy if the review is completed by then.

@vdegenne
Copy link

@alvaromontoro What is the status on this? It's been 1 year already.
I don't think you need to "thoroughly review all the code" as it is just a TypeScript port (to generate typings), @JumpLink probably took the time and effort to rewrite your code and adding types actually reduces the chance of having major breaks.
If it's ok, you just have to run your existing tests and if everything is ok it would be nice to bump the update on npm!

@vdegenne
Copy link

vdegenne commented Feb 13, 2023

@alvaromontoro If you are not sure about how this update will play out, you could accept the changes on a separator branch (e.g. pre-typescript) so your main branch remains unchanged.
And then push this typescript version onto npm using the same tag so we can install it using a syntax like

npm i -D gamecontroller.js@pre-typescript

Note: Another big advantage of TypeScript is that you can deliver the doc directly into the declaration files (.d.ts),
So for instance if you start typing gamepad.on(' your IDE will show you the available events and assuming we add the documentation into the declaration file we could know for instance that button0 is A on the Xbox controller and so on (without the need to always go back and forth between code and controller map image.)

@vdegenne
Copy link

vdegenne commented Jun 1, 2023

@JumpLink By the way, it might help the review if you remove the dist directory from this PR.
It's not recommended to stage production files in the repo, if you include the build steps in the package.json then the author can generate the dist files on his end and publish them with npm publish.
Please consider doing that as soon as you can, so maybe it may help the author to look and accept this PR or not.

@vdegenne
Copy link

vdegenne commented Jun 1, 2023

@JumpLink Ok I couldn't resist to make my own esm version https://www.npmjs.com/package/esm-gamecontroller.js

If you are interested in making some changes, let's continue the work there!

Have a good one.

@josheverett
Copy link

@vdegenne thank you for this!

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.

4 participants