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 and jest #64

Closed
wants to merge 9 commits into from
Closed

Conversation

HarelM
Copy link
Contributor

@HarelM HarelM commented Apr 24, 2024

This is basically doing most of the heavy lifting.
There might be some cleaning up to do regarding grunt and jasmine, but I think most of the heavy lifting is done as part of this PR.
Fixes #36
Fixes #24

The files are generated in the lib folder to be used when publishing to npm.
e2e seems to work so I didn't see a backwards compatibility issues in terms of public API.
Not sure I'm super excited about the inheritance part, as I prefer composition over inheritance, but I think I managed to change it correctly.

Any feedback is welcome.

To build run npm run build
To run tests run npm run test

There are also some packages that can probably be removed like lodash, rimraf, make-dir that can be easily done using a one liner I believe.

@davidparsson
Copy link
Owner

Brilliant, thanks a lot! I'll try to review as soon as possible.

I might split up the initial commit into two, one that modifies everything but the tests and one that modifies the tests, to make sure that the old tests still passes with the new code.

Also, it seems the tests don't pass as is. Are you able to take a look?

@HarelM
Copy link
Contributor Author

HarelM commented Apr 25, 2024

Sure, I'll take a look.

@HarelM
Copy link
Contributor Author

HarelM commented Apr 25, 2024

A change I did before I pushed that broke the build, sorry, my mistake, should be OK now.

@HarelM
Copy link
Contributor Author

HarelM commented Apr 29, 2024

Seems like old node version are failing due to syntax issue.
I would consider removing those unsupported versions from the CI.
Seems like node 20 is passing the tests.

@davidparsson
Copy link
Owner

davidparsson commented Apr 30, 2024

Right! I was hoping to make this change non-breaking, but dropping support for node.js versions have been unsupported for years seems reasonable.

However this seems to be caused by tsc requiring node.js 14.17+ to compile. I'm not sure it's even worth investigating, but it might still be possible to produce JavaScript that runs on the old versions.

@HarelM
Copy link
Contributor Author

HarelM commented Apr 30, 2024

Nodejs 12 is almost 5 years old and is probably considered a security hazard.
I think supporting nodejs from the last 3 years is enough...
Your call. In my OSS projects I support LTS and a version back - i.e. node 20 and node 18 in this case.

@davidparsson
Copy link
Owner

I have merged another PR that dropped support for node.js 14 and older, to main. Once this branch has those changes and the tests pass I'll take a closer look at your changes.

Thanks again for your help, and your patience!

@HarelM
Copy link
Contributor Author

HarelM commented May 1, 2024

I've updated my branch, but you still need to allow the tests to run...

@HarelM
Copy link
Contributor Author

HarelM commented May 1, 2024

BTW, I've configured my projects to run PRs for people that are not new to github, this solves most of the need to click everytime someone commits a change...

@davidparsson
Copy link
Owner

Thanks! That makes sense. I've done that as well now.

@davidparsson
Copy link
Owner

I've been very busy the last few weeks, but I haven't forgotten about this. I found the need to make a few adjustments, but since I did not have write access to this PR i made a new one in #66.

@HarelM
Copy link
Contributor Author

HarelM commented May 14, 2024

Do I need to enable any settings in my repo so you'd have write access?
Feel free to use this code and modify it.
Close this PR is needed too.
Thanks!

@davidparsson
Copy link
Owner

Closing this in favor of #66, which contains these changes. I'm still very busy but I'll try to get this reviewed as soon as possible. Thanks!

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.

Migrate from grunt to npm scripts Consider adding typescript definitions
2 participants