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

Code Modernization #57

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

Code Modernization #57

wants to merge 6 commits into from

Conversation

jens-ox
Copy link
Contributor

@jens-ox jens-ox commented Nov 22, 2023

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:

  • all files migrated to ES2015 classes
  • code moved to ES Modules
  • externally available file decoders used instead of inlining them in lib
  • proper linting setup
  • linting setup added to CI

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.

@jens-ox jens-ox changed the title wip Code Modernization Nov 22, 2023
@hanayik
Copy link
Collaborator

hanayik commented Nov 22, 2023 via email

@rii-mango
Copy link
Owner

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.

@rii-mango
Copy link
Owner

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?

@jens-ox
Copy link
Contributor Author

jens-ox commented Jan 25, 2024

Hi @rii-mango! How can I reproduce this? npm run build and npm run test both work fine for me.

@rii-mango
Copy link
Owner

@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:
rii-mango/JPEGLosslessDecoderJS#18
#58

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?

Copy link

@niks2060 niks2060 left a 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
Copy link
Owner

@niks2060 - after @jens-ox's PR that updated JPEGLosslessDecoderJS to TypeScript, there is no longer a release folder and people are unable to figure out how to use it. Will this PR for Daikon be the same?

@jens-ox
Copy link
Contributor Author

jens-ox commented Mar 25, 2024

@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 JPEGLosslessDecoderJS). In particular, I missed that users had the option to directly download the library from GitHub.

I'll open a PR for JPEGLosslessDecoderJS to make sure users can import it as expected.

Edit: Opened a PR for JPEGLosslessDecoderJS. I will update this PR tomorrow to ensure that the output files are where they're expected.

@niks2060
Copy link

@niks2060 - after @jens-ox's PR that updated JPEGLosslessDecoderJS to TypeScript, there is no longer a release folder and people are unable to figure out how to use it. Will this PR for Daikon be the same?

I think we need to work on global access like webpackUniversalModuleDefinition and I will try to add same in JPEGLosslessDecoderJS and give it a try.

@jens-ox
Copy link
Contributor Author

jens-ox commented Mar 26, 2024

I think we need to work on global access like webpackUniversalModuleDefinition and I will try to add same in JPEGLosslessDecoderJS and give it a try.

@niks2060 not sure what you mean by that. I would recommend sticking to ES Modules and only compile to UMD if absolutely needed.

@niks2060
Copy link

@jens-ox I have worked on cornerstone and they have implemented same way using UMD.

@jens-ox
Copy link
Contributor Author

jens-ox commented Mar 26, 2024

@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.

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