wampy.js icon indicating copy to clipboard operation
wampy.js copied to clipboard

My full and complete TypeScript refactor

Open MakeShiftArtist opened this issue 3 years ago • 7 comments
trafficstars

Description, Motivation and Context

I first decided to do a TypeScript refactor when I noticed that the library had outdated and incomplete typings at DefinitelyTyped. So, I went through the process of refactoring the entire module in typescript, using strict, so that it's very thorough. I'm still unable to run tests myself, but I have made sure the library still works in my own project. While this is a massive update and does include some changes to the actual code, hardly any is actually code related. Almost all changes relate to typings and documentation. The library in its current form is not very developer friendly, not allowing you to extend the Wampy class with full IDE intellisense. While the module is still "usable" it's far from perfect. While this refactor doesn't make it perfect, it greatly improves the readability, while also making it a lot less error prone due to TypeScript error checking. I feel like this will be a massive benefit to the module as a whole and could greatly improve the developer experience.

The solves many issues (most are closed) but it does solve the issue #168 I posted earlier

What is the current behavior?

Currently, the entire module is written in Javascript and typed with JSDocs. While JSDoc is better than nothing, it's nowhere near as typesafe, and self documenting as TypeScript with TSDoc. The only actual typings are made by the community, are outdated, and in some cases flat wrong.

What is the new behavior?

Full TypeScript support, better documentation, better type annotations, better readability, and some minor code improvements/bug fixes. Potentially breaking code change is in the method _wsOnError where the parameter error is passed directly, instead of { error } for API consistency.

Exports are now more robust, instead of just exporting the Wampy class. You can now manually import the constants and the built in serializers, for better developer friendliness

What kind of change does this PR introduce?

  • [X] Bug fix (non-breaking change which fixes an issue)
  • [X] New feature (non-breaking change which adds functionality)
  • [X] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [ ] My code follows the code style of this project.
  • [ ] I have added tests to cover my changes.
  • [X] Overall test coverage is not decreased.
  • [ ] All new and existing tests passed.
  • [X] My change requires a change to the documentation.
  • [X] I have updated the documentation accordingly.

MakeShiftArtist avatar Apr 15 '22 03:04 MakeShiftArtist

Wow! Huge work! Thanks @MakeShiftArtist ! I need some time to review. I've enabled ci. So you can now see check results. Seems that something is broken.

KSDaemon avatar Apr 23 '22 17:04 KSDaemon

First comment-question: i see a map file for every source file. Why are they here? I mean under the source control? I think they are just for debuging a prod/dist version. And should be only there.

KSDaemon avatar Apr 23 '22 17:04 KSDaemon

Also as sources are now in typescript, we should adopt ci and test runner process.

KSDaemon avatar Apr 23 '22 17:04 KSDaemon

Wow! Huge work! Thanks @MakeShiftArtist ! I need some time to review. I've enabled ci. So you can now see check results. Seems that something is broken.

Run npm test

> wampy@6.[4](https://github.com/KSDaemon/wampy.js/runs/6141549331?check_suite_focus=true#step:8:4).2 test /home/runner/work/wampy.js/wampy.js
> npm run test:node && npm run test:browser


> wampy@[6](https://github.com/KSDaemon/wampy.js/runs/6141549331?check_suite_focus=true#step:8:6).4.2 test:node /home/runner/work/wampy.js/wampy.js
> NODE_ENV=test nyc ./node_modules/mocha/bin/mocha --exit --require @babel/register -R spec

(node:2043) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

/home/runner/work/wampy.js/wampy.js/src/serializers/JsonSerializer.ts:1
import { Dict, Serializer } from "../typedefs";
^^^^^^

SyntaxError: Cannot use import statement outside a module

Looks like tests aren't running them as TypeScript files. I'm not sure how tests are configured in this module, but switching tests to the dist folder may solve this

MakeShiftArtist avatar May 15 '22 16:05 MakeShiftArtist

First comment-question: i see a map file for every source file. Why are they here? I mean under the source control? I think they are just for debuging a prod/dist version. And should be only there.

My experience with TypeScript and its configuration is somewhat limited, so if I had the map files in the wrong location, or in unnecessary areas, I do apologize. I'm trying to migrate over to TypeScript at the moment so I may make some config mistakes here or there.

MakeShiftArtist avatar May 15 '22 16:05 MakeShiftArtist

Also as sources are now in typescript, we should adopt ci and test runner process.

I agree, but am not experienced enough with this process to give much insight. I'm just getting into test driven development.

MakeShiftArtist avatar May 15 '22 16:05 MakeShiftArtist

Reviewing some of the failed checks, it appears that the checks are still expecting JavaScript code but failing due to it checking the source files instead of the dist files. I'm not sure how to change that, if I even can

MakeShiftArtist avatar May 25 '22 17:05 MakeShiftArtist

Hi! I've refactored heavily the whole project. So now this PR can not be merged either, too many conflicts. So closing for now.

KSDaemon avatar Oct 06 '22 07:10 KSDaemon