-
Notifications
You must be signed in to change notification settings - Fork 55
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
Code Modernization #57
base: master
Are you sure you want to change the base?
Conversation
Hi @jens-ox - I inherited this account when the previous owner took a new job. I don't have much JS/ES experience and depend on contributions to keep things current. So I'll trust your and @hanayik opinions on this. Regarding downstream projects, that's Papaya which is using a fragile custom Papaya-Builder that needs to be replaced with something more modern. I can set Papaya to reference the previous version of Daikon if needed before a newer build system is set up. |
Hi @jens-ox, thanks for the pull requests. I have accepted the one in the JPEGLosslessDecoderJS repository and updated the version to 2.1.0 in GitHub and npm. However, when I try to build using the PR for Daikon, I'm getting missing library errors for CharLS and jpeg2000. Do you know what's going wrong there? |
Hi @rii-mango! How can I reproduce this? |
@jens-ox I thought JPEGLosslessDecoderJS was working well, but apparently I didn't test it well enough. Some people have been chiming in on a couple of issues with it: It looks like there is no build or release folder in the repository anymore. Is the intended change to use build/main.cjs as the release version of lossless.js? Or is it lossless.ts now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved after reviewing changes but need to resolved merge conflict and above comments.
@rii-mango @niks2060 I wouldn't merge this and expect nothing to break - it's more or less a full refactor after all, and code produced by the TypeScript compiler won't be structurally identical to the code before. I'm sorry for not stating that in this PR (and the one in I'll open a PR for Edit: Opened a PR for |
I think we need to work on global access like |
@niks2060 not sure what you mean by that. I would recommend sticking to ES Modules and only compile to UMD if absolutely needed. |
@jens-ox I have worked on cornerstone and they have implemented same way using UMD. |
@niks2060 if that aligns with downstream usage of Daikon, then that might be a good choice. Still, aside from ES modules being the official module standard, using ES modules instead of UMD has many advantages. I can recommend Mozilla's deep dive into ES modules in case you're unfamiliar with them. |
I initially only wanted to set up proper linting, but wasn't able to find a decent middle-ground without doing a full refactor.
The following things have been done:
lib
I know this is more or less an un-reviewable PR - if it's preferred, I can also release this fork as a separate package.
Here is the latest run of the CI workflow.