cornerstonejs / cornerstone

JavaScript library to display interactive medical images including but not limited to DICOM
https://docs.cornerstonejs.org/
MIT License
2.05k stars 597 forks source link

Splitting the library into modules and removing jquery #92

Closed Revln9 closed 6 years ago

Revln9 commented 7 years ago

Some of us would like to see a minimal and a decoupled version of this library without the jquery dependency and other shiny stuffs (tools, handlers...) .

this will open up the road for multiple frameworks and Gui development. i'm using react and started about a week ago trying to figure out how to approach this to make a react wrapper around it , and its seems like this library was written with strong presemptions (jquery ) .

i think that using a 250 kb library (jquery ) just for handling events and a couple of ui interactions is a waste , event attachments can be done in vanilla javascript and its dead simple

var eventData = { element : element };
var event1 = new CustomEvent("CornerstoneElementDisabled", eventData);
     element.dispatchEvent(event1);

the core of this library can be more "modular" with different npm packages , each one doing nothing else than its own work ( no props passing...) . This will bring more people into it. and this will allow to other people to write plugins and modules for it.

documentation needs to be clarified with a good and detailed tutorial with the bare minimal requirements (cornerstone, codecs , wado loader ) no jquery, no tools , nothing else , just the code to transform a dicom into an array of pixels and display it on a canvas.

An example of a dead simple tool module (without jquery ) would be helpful too.

Of course most of us could decouple this in a couple of days , but the guys who wrote this can do it 10 times faster .

And i would like to thank you all , especially @chafey for the huge amount of work he has given to the community.

jpambrun commented 7 years ago

I strongly agree with removing jQuery as a dependency and I would be happy to help review any PR on this topic.

On CustomEvent, looks like IE11 must shimmed[0] a bit and older versions are not supported (is this a problem? do we need support for IE10 and bellow?).

JF

[0] http://caniuse.com/#feat=customevent

On Sat, Feb 25, 2017 at 1:56 AM Revln9 notifications@github.com wrote:

Some of us would like to see a minimal and a decoupled version of this library without the jquery dependency and other shiny stuffs (tools, handlers...) .

this will open up the road for multiple frameworks and Gui development. i'm using react and started about a week ago trying to figure out how to approach this to make a react wrapper around it , and its seems like this library was written with strong presemptions (jquery ) .

i think that using a 250 kb library (jquery ) just for handling events is a waste , event attachments can be done in vanilla javascript and its dead simple

var eventData = { element : element }; var event1 = new CustomEvent("CornerstoneElementDisabled", eventData); element.dispatchEvent(event1);

the core of this library can be "modularized" in different npm packages , each one serving specific purpose example : cornerstone-core for only the core , cornerstone-tools... , cornerstone-viewers..., this will bring more people into it. and this will allow to other people to write plugins and modules for it.

documentation needs to be clarified with a good and detailed noobie tutorial for each section , it will make this a bit clearer ,

And i would like to thank you all , especially @chafey https://github.com/chafey for the huge amount of work he has given to the community.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/chafey/cornerstone/issues/92, or mute the thread https://github.com/notifications/unsubscribe-auth/AAr2MHFbjGXxsVmKYn_pklVr35UfEL4Cks5rf9CtgaJpZM4ML7ie .

Revln9 commented 7 years ago

Actually i was able to decouple the cornerstone core and the wadoloader from the jquery library , and it wasn't easy , i am at the final stage where i recieve the image element and related data but seems like the method cornerstone.displayImage broke too so , i'll dig deeper tonight , and hopefully tomorrow , i'll make a fork

in fact jquery was used for promise handling too , and since everything was 'deferred' , it was a bit tricky to rewrite in es6 , especially the webworker promises for the wadoloader.

jpambrun commented 7 years ago

oh I see. This deferred dependency may be an issue.

@swederik jQuery is a pain point for me as well. Is there a solution for the deferred requirement?

jpambrun commented 7 years ago

We could use a native deferred implementation or a use a very small polyfill with reject support (this?).

Also, we will probably need a polyfill for custom event (this?).

Ideally, we would keep the dependencies to a minimum..

swederik commented 7 years ago

I also agree with removing the jQuery dependency, but it just hasn't been a priority thus far. @jpambrun is right, as long as we have reject support, we can use anything.

I'd be happy to help review a PR for this!

Revln9 commented 7 years ago

Got it working check it at https://github.com/Revln9/react-cornerstone , i don't really know how to make a PR so if anyone has time...otherwise i'll try to make one this week.

Note that i used Es6 Promises and the method DIspatchEvent so , it will be hard to port it on anything else than chrome .

There's a library called es6 promises which is a polyfill (i think) or we could use something just like bluebird

Another important point is the usage of the webworkers , Which can cause problems on some platforms (electron) , so we need to add an on / off switch at the top level library which tells to the image loaders and other plugins if they are allowed to use webworkers or not.

Suggestions and thoughts are strongly encouraged

Revln9 commented 7 years ago

@jpambrun @swederik any update on this ?

jpambrun commented 7 years ago

I think it's on the way? @swederik, can you give us an idea of what is left?

swederik commented 7 years ago

It's mostly a question of finding time to do the transition and not break other libraries.

There is a branch (https://github.com/chafey/cornerstone/tree/zachasme-es6) that has jQuery removed and seems to work fine. Likewise, there is a branch of CornerstoneTools (https://github.com/chafey/cornerstoneTools/tree/noJquery) which has been updated to listen to CustomEvents from Cornerstone instead of using on/off in jQuery. I need to rebase both of these again before I could merge them. I also want to finish the CornerstoneTools migration to ES6 modules first.

The main reasons I have held off for now are that:

Revln9 commented 7 years ago

Yes , good point , no breaking change , and some documentation about it. the branch seems to be clean , now we just have to be sure that the polyfills works correctly and the tools (with jquery) works fine too.

I think you need to announce it before the release . concerning the tools , we're all aware that it will be harder to avoid the use of jquery since its heavily dependent on it. but we can keep it that way for now, and rewrite / release tool by tool later . it will also help to decouple the tools , which means they will be easier to maintain (bug tracking , issues, releases...).

dannyrb commented 6 years ago

The bulk of this work seems to have transitioned to the event and deferred issues that are currently being worked on. I'm going to close this. Feel free to monitor:

For related progress. To varying degrees, some of the jQuery work mentioned here has already been completed.