search

rii-mango / Daikon

A JavaScript DICOM reader.
Other
221 stars 54 forks source link

Code Modernization #57

Open jens-ox opened 12 months ago

jens-ox commented 12 months ago

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:

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.

hanayik commented 12 months ago

I personally don’t think it should be a separate package. The NiiVue team have tried our best to contribute back to the projects we depend on when we can, rather than releasing modified packages under a new scope. I think we can get this stuff merged in, but it would be good to get some feedback from @rii-mango about any unintended consequences in downstream Daikon software.

From: Jens Ochsenmeier @.> Date: Wednesday, 22 November 2023 at 11:14 To: rii-mango/Daikon @.> Cc: Subscribed @.***> Subject: [rii-mango/Daikon] wip (PR #57)

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:

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.

You can view, comment on, or merge this pull request online at:

https://github.com/rii-mango/Daikon/pull/57

Commit Summary

File Changes

(35 fileshttps://github.com/rii-mango/Daikon/pull/57/files)

Patch Links:

— Reply to this email directly, view it on GitHubhttps://github.com/rii-mango/Daikon/pull/57, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABHZ3OBX47H55PKRGU2AQ2TYFXNBZAVCNFSM6AAAAAA7WBWFM6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGAYDMMJQHA3TGNA. You are receiving this because you are subscribed to this thread.Message ID: @.***>

rii-mango commented 11 months ago

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 commented 10 months ago

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 commented 9 months ago

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

rii-mango commented 9 months ago

@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: https://github.com/rii-mango/JPEGLosslessDecoderJS/issues/18 https://github.com/rii-mango/Daikon/issues/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?

rii-mango commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

jens-ox commented 7 months ago

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