node-red / node-red-dashboard

A dashboard UI for Node-RED
Other
1.32k stars 453 forks source link

update to latest gridstack.js #569

Open adumesny opened 4 years ago

adumesny commented 4 years ago

noticed from your package.json that you are using "gridstack": "^0.4.0"

which is VERY old - might want to update to the latest (1.1.0) or at least 0.6.5 since that lib is being maintained. Ping me if you have any questions.

dceejay commented 4 years ago

have you tried upgrading ? does the layout capability still function ?

adumesny commented 4 years ago

I'm not using this lib, but the maintainer of gridstack. thank you.

dceejay commented 4 years ago

Ok a quick test shows that gulp no longer builds it even with version 0.5 ( something about singular glob scss file missing.) so not an easy upgrade for something that isn’t broken.

Error: File not found with singular glob: /Users/foo/Projects/node-red-dashboard/node_modules/gridstack/src/gridstack-extra.scss (if this was purposeful, use `allowEmpty` option)

we need it as we default to 30 columns... and I see the src has been removed from the npm package... so we may be stuck on 0.4.0 for a while

(EDIT _ forgive me - the src is still there in 0.5.0 so we could go to there at least)

dceejay commented 4 years ago

hmm - removing jquery would be nice (moving to 1.x) - but any clues how to load that scss file via npm ?

adumesny commented 4 years ago

that scss hasn't changed in years so you could just have it locally (ideally just generate 30 columns not 1-30 which is a lot of styles IFF you don't let users change the column number). not sure why NPM included the source code files in the past!

How were you setting $gridstack-columns to 30 anyway ? didn't you have to modify ? I suppose I could include in the dist/ npm package since I mention using in doc for custom columns but hasn't looked how people use it during builds...

Also since 0.4.0 there has been 300+ commits since then - surely something you could use :) for one thing jquery-ui I bundle is 1/5th the original size! https://github.com/gridstack/gridstack.js/releases/tag/v0.4.0

as for removing jquery, it's work in progress. In the1.0 API it is removed so you can switch now and it will go away as a load in the future...

Also how come gridstack is not a package dependency but a dev dependency ? I see you had 75k download a year ago but gridstack was more like 8k back then so wondering how that's possible... in case you are wondering, I'm trying to show more actual usage of gridstack so more people use it and contribute back.

dceejay commented 4 years ago

We run it through gulp - so that did the sass thing to build the .min.css for us. We load all extra libs as dev deps and then gulp does the build for us to just copy in the relevant files into our dist.

OK I guess we'll just grab that one file manually and add it to the build - see what happens :-) hmm though a quick look says the latest scss has change just 10 days ago...so I don't know if it's backwards compatible.

So far I can get to 0.5.5 ok - trying 0.6 breaks something in the drag drop between groups - seems to create duplicates that are ephemeral and cant be interacted with.

adumesny commented 4 years ago

yeah I changed it to generate 2-11 by default since 12 and 1 are already done in the default css. no biggy... Were you able to pass in $gridstack-columns without changing the file itself ?

6.x sends a LOT LESS events (only what changed) so if you just add a new node you will get a single added event no change event if nothing else on the grid changed. maybe you depended on that broken behavior ? just listen to added/deleted too.

would be great if you could external depend on gridstack instead of compiling it. odd your package compiles everything in instead... user have no choice of a more recent npm build, etc...

you should also just include gridtsack.all.js and .css as that include jquery/jquery-ui trimmed down versions.

dceejay commented 4 years ago

currently the gulp function does this - happy to take advice to update

function gridstack() {
    gulp.src('node_modules/gridstack/dist/gridstack.min.css').pipe(gulp.dest('dist/css/'));
    gulp.src('node_modules/gridstack/dist/gridstack.jQueryUI.min.js').pipe(gulp.dest('dist/js/'));
    gulp.src('node_modules/gridstack/dist/gridstack.min.js').pipe(gulp.dest('dist/js/'));
    gulp.src('node_modules/gridstack/dist/gridstack.min.map').pipe(gulp.dest('dist/js/'));
    gulp.src('node_modules/lodash/lodash.min.js').pipe(gulp.dest('dist/js/'));
    return gulp.src('src/gridstack-extra.scss')
        .pipe(replace('$gridstack-columns: 12 !default;','$gridstack-columns: 30;'))
        .pipe(sass({outputStyle: 'compressed'}))
        .pipe(rename({extname: '.min.css'}))
        .pipe(gulp.dest('dist/css'))
}

and we have to fix/pin it- precisely in order not to break when you upgrade things ! Users don't want choice - they want it to work,

adumesny commented 4 years ago

that's why you have npm 0.5.x vs 0.6.x vs 1.x - those can be breaking changes! you saying your lib never ever breaks things for your users ?

I see about the gulp replace... neat. guess for now include a copy of the latest, and maybe I will export in next release again. that would be v1.1.1 if you need it.

as for the script it should for 0.6.4 / 1.x:

function gridstack() {
    gulp.src('node_modules/gridstack/dist/gridstack.min.css').pipe(gulp.dest('dist/css/'));
    gulp.src('node_modules/gridstack/dist/gridstack.all.js').pipe(gulp.dest('dist/js/'));
    return gulp.src('src/gridstack-extra.scss')
        .pipe(replace('$gridstack-columns: 11 !default;','$gridstack-columns: 30;'))
        .pipe(sass({outputStyle: 'compressed'}))
        .pipe(rename({extname: '.min.css'}))
        .pipe(gulp.dest('dist/css'))
}

we NO LONGER use lodash (450k!) and jquery/jquery-ui trimmed are part of all.js (which will get smaller over time wihout changing your needs) so don't include those elsewhere

adumesny commented 4 years ago

I will be happy to help you with making gridstack an external dependency (with fixed range you can control what works). this way users can update a module (within spec or force it outside) if they so desire because it releases at a different schedule than you do... if a bug fix dot release comes out, they can't have it until you rev - and won't know since you hide what you use... This is the reason you've been using a 2 years old release that is 300 commits behind... think about it. This wouldn't work enterprise software (what I do) where you need to know your open source dependencies AND do security patches often.

dceejay commented 4 years ago

Well in theory changes from 0.5 -> 0.6 should not really include breaking changes... so removing events is a bad idea... but yes if below version 1 they are allowed (within semantic rules:-( ).

Enterprise "customers" have plenty more resources than the typical open source project and can quite easily inspect the package.json and license files of any project using tools like https://libraries.io/npm/node-red-dashboard - and it would be great if they offered up help to fix anything they may want to rely on.

I'll try those gulp changes and see how we go. Thanks.

adumesny commented 4 years ago

are you hard-coding to 30 columns or letting the customer pick (or change based on screen resolution - because in 0.6.x I made setColumn(N) work A LOT better for responsive design. The reason I'm asking is because we could modify the extra.css to only generate for column=30 (I can add a start number) which would make the CSS file a LOT smaller and more efficient than having 30+29+28+27+... sets of styles.

dceejay commented 4 years ago

Most of the time they would be using 6 or so... maybe 9, 12 etc... but there is nothing to stop a user going larger if they wanted to... (indeed I'm not sure why the limit is 30 :-).

So far experiments show that there is indeed a lot of breakage trying to goto 1.0 - so i think we'll aim for 0.6.x - though I'm even struggling to see why that is failing. It seems to be when we drop a cell into a new group and it has to auto-resize... it seems to create a new smaller cell (correct) but leave the old larger one... which then causes other errors of course.

image

Is there a list of methods that changed going to 0.6 ?

adumesny commented 4 years ago

no api change in 0.6.x, just event changes and implementation of setColumn(N) which was experimental (and not working well at all).

Are you including jquery/jquery-ui yourself or using ours ? 1.x has it included in gridtsack.all.js, which older build have it separate. When did this behavior issue started appearing ?

dceejay commented 4 years ago

I thought it may be as I was trying to use setColumn... have now reverted to setGridWidth and it's still the same behaviour on 0.6.4 - Is ok on 0.5.5. But seeing as 0.5.5 just removes the scss src I'm not sure there is much benefit for us to move there.

Thanks for the fix above - but we are a long way from jumping to that yet :-)

adumesny commented 4 years ago

ok, let me know if I can help getting you to use an external dependency instead of compiling things in.

dceejay commented 4 years ago

Sure - I'm still not sure how that helps anything. We need to serve those files up - so we need them in a place we know (our dist folder). We can't rely on the vagaries of npm to put them somewhere else (nor do we want to add more paths to express to serve them) - so we need to copy them over somewhere - and as we need to do the sass thing we may as well do that at build time ??? what am I missing ? How does making it external help ?

adumesny commented 4 years ago

the build (happening on the server) would do the npm install and compile to serve to final bits - typically packages list other packages as external dependencies - this way anybody using your lib can see what's being used (in yarn.lock or npm tools) and can rev to a later bug fix if necessary without waiting for you to release something. That problem gets worse if you have many nested lib of course, and you may end up with multiple copies to same lib. That's why there is a 'resolve' field in package.json as well. The other personal side, is that you had >80k/week downloads last year, yet my total download of gridstack was <8k which clearly doesn't show dependency usage...

adumesny commented 4 years ago

hey is that a SF golden gate bridge picture (my backyard kind off) - you in UK so wondering..

dceejay commented 4 years ago

Yes - managed to get out there for a trip while back. Drove up Highway 1 etc.. I can see your point re downloads... but getting our users to download extra files they don't need won't help as they won't be in the right place for us to serve - so if/when they upgrade gridstack it wouldn't touch the one we had built in at build time.

I must be still missing something... how does our html page know where to get the right gridstack.min.js file from if it's now outside of our served path (/dist) ? (As npm may put it under our project - or it may not... sometimes it puts things in a higher up (flattened) node_modules dir etc... When you said resolve field ... did you mean resolutions ?? that is a yarn only thing... we use npm.

And if users could upgrade it as you suggest - they could indeed get your fixes... but would break dashboard - as indeed it does now :-) . I'd love to be able just to move to latest, but so far I've been trying and spent over a day not getting far and can't really afford that sort of time.

All the code is being used in https://github.com/node-red/node-red-dashboard/blob/master/nodes/ui_base.html - mainly around line 700 +/- If you can see anything obvious please do shout.

knolleary commented 4 years ago

I think need to separate out the different issues here.

Clearly upgrading to the latest gridstack requires some unknown amount of development effort. I would suggest we don't conflate that with trying to get a the point where we have gridstack as a regular dependency rather than just a devDependency. Would could do the latter even with the version pinned in package.json to the exact version we use today. And once we have that working, then look at the work needed to upgrade.

@adumesny the key thing for us is we do not want to have to introduce any sort of build step as part of npm install - we don't have that today and to introduce one would require adding a considerable amount of additional dependencies that our end users would have no direct use of.

@dceejay assuming the npm install of gridstack included the file we needed already built, then we could do something at startup to copy the file from the gridstack module directory (as located by require.resolve("gridstack") into our dist folder. Its a bit hacky, but would keep things working and could be iterated on/improved.

adumesny commented 4 years ago

I would suggest we don't conflate that with trying to get a the point where we have gridstack as a regular dependency rather than just a devDependency. Would could do the latter even with the version pinned in package.json to the exact version we use today.

Correct, When your lib users run npm install that would also get the pinned gridstack image. When your code has require.resolve("gridstack") (which should be gridstack.all.js to get everything as one JS file) that will automatically get the node module directory installed version, so there is no need for you to also copy those files to your dir. The only thing you are generating is a 30 column css file (you can keep on doing that by having your build script and copying to your dist - you could rename it as 30_column or something as well and include that in your css files.

if 0.5.5 works I would highly recommend trying that as the lib is A LOT smaller (-450k lodash, -200k jquery-ui unless you are also using them and need the full version anyway)

dceejay commented 4 years ago

but we don't require it anywhere... we load it via ajax call to the backend... https://github.com/node-red/node-red-dashboard/blob/master/nodes/ui_base.html#L383

knolleary commented 4 years ago

@dceejay We don't require it today because we don't npm install it as a dependency. Chicken meet egg.

The point is we could require it in ui_base.js where it sets up all the http routes for serving the dashboard resources. Its two lines of code to add a route for the gridstack file to be served from its npm module directory - a location the code can identify using require.resolve("gridstack");.

(I realise my suggestion of copying the file over is a non-starter because there's no guarantee the user running the code would have file-write permission in the module directory.)

dceejay commented 4 years ago

Worth a try I suppose - users get to install 2.4 MB of goodness (of which 1.6M is indeed jquery we hope to lose) vs 110k today.

adumesny commented 4 years ago

v0.5.5 npm is 625k unpacked and that includes jquery-ui (full + min) which you had to npm install before (and much larger lib at runtime). Later version include jquery as well (as we hide it as part of our transition away). install space is cheap, runtime is not (what I fixed after 0.4.0) https://www.npmjs.com/package/gridstack/v/0.5.5

the 0.4.0 you had was 425k (without external dependencies including very large lodash), but yeah only you had to install to build. Not sure where you're getting 2.4mb of install stuff.

Noteworthy changes (and many fixes)

v0.5.x lodash removal (450k), minimal jquery-ui.js (55k vs 248k) v0.6.x huge responsive improvements with implementation of setColumn(N) and oneColumnMode options. Also much fewer unnecessary change callbacks v1.x jquery was removed from the API and external dependencies (still used internally during transition). see https://github.com/gridstack/gridstack.js#migrating-to-v100

dceejay commented 4 years ago

apologies - I have 0.5.4 installed

1.6M    ./jquery
804K    ./gridstack

1.6 + 0.8 = 2.4. but yes 0.5.5 is 652k

and yes absolutely I want to move - but I can't even get 0.6 to behave properly with those changes let alone 1.x

adumesny commented 4 years ago

got it. But really who cares about disk space to build (cheap). runtime compile size is much more important.

Also wondering do you know why node-red-dashboard went from 60k a year ago (seem to recall seeing 80+k) to just 17k now ? https://www.npmjs.com/package/node-red-dashboard

dceejay commented 4 years ago

but it's not just to build - it is there unused on every users device. We do have people installing stuff over slow links. OK - it's not a runtime hit, and disk is cheap, but it isn't needed. "Who cares ?" was my generations thinking that got the planet into the state it is...

I'll try Nick's suggestion to see if I can get loading working that way - and see how we go from there.

dceejay commented 4 years ago

OK that seems to work. (at 0.5.5), locally. pushed to master to test. Feel free to update gridstack further :-)

dceejay commented 4 years ago

so is the setColumn in 0.5.5 OK to use ?

adumesny commented 4 years ago

setColumn is just a rename of setGridWidth to better represent what it actually is. it's better than OK. it actually works now!

dceejay commented 4 years ago

Now meaning 0.5.5 and not 1.1.0 ?

adumesny commented 4 years ago

you'll want 0.6.x and later for better setColumn. That's when I re-wrote it. Especially the 1 column mode responsive mode. check the release notes https://github.com/gridstack/gridstack.js/releases

adumesny commented 4 years ago

hey I see you got gridstack v0.5.5 as external dependencies (yeah!) and removed lodash load. great. can't wait to see the download changes when you release it.

let me know if you want help later migrating to a more recent version LATER. 0.6.x shouldn't be that hard (might need to add added|removed events. 1.x is more how you init and access gridstack var, but again minimal API. removing your jquery dependencies (>350 instances in just 1 on your file) would be a different story :(

dceejay commented 4 years ago

Hi Thanks - I have asked the original developers of this node (Hitachi) to take a look at the 0.6/1.0 piece as I can't seem to find the magic. Is definitely something to do with adding and removing but I'm just not seeing it.

Yes we use jquery a lot so we won't be removing it - but if we can avoid the duplication that would good to do.

adumesny commented 4 years ago

@dceejay any chance I could have your company logo (IBM, red project, etc...) added to the used by section (bottom) of https://gridstackjs.com/ ?

knolleary commented 4 years ago

Hi @adumesny. You are more than welcome to use the Node-RED logo - https://nodered.org/about/resources/

It would not be appropriate to use the IBM logo.

adumesny commented 4 years ago

done. thanks!

HiroyasuNishiyama commented 4 years ago

So far experiments show that there is indeed a lot of breakage trying to goto 1.0 - so i think we'll aim for 0.6.x - though I'm even struggling to see why that is failing. It seems to be when we drop a cell into a new group and it has to auto-resize... it seems to create a new smaller cell (correct) but leave the old larger one... which then causes other errors of course.

image

Is there a list of methods that changed going to 0.6 ?

This problem seems to be caused by a change of removeWidget method in https://github.com/gridstack/gridstack.js/pull/1097 between gridstack v0.5.5 to v0.6.0.

This PR deleted following DOM element removal code from removeWidget.

    if (detachNode) {
      el.remove();
    }

This caused a cell belonging to original grid left after its move.

@adumesny Is this an expected API change?

adumesny commented 4 years ago

@HiroyasuNishiyama hey thanks for tracking this down - to be honest I can't recall now why I removed this (I remember being confused with the reverse logic) but doing a quick look the DOM node should get removed further down in the grid engine removeNode() callback which does:

        if (detachNode && n._id === null) {
          if (n.el) {
            $(n.el).remove();
          }

which you step into when running https://gridstackjs.com/demo/column.html and clicking a widget X button. Would be great to get a running jsfiddle example showing the issue so I can debug it.

Also how are you guys calling removeWidget() during a drop ? when doing drag&drop between grids, that method isn't called at all. The drop (and leaving dragout) event handlers are doing the work. https://gridstackjs.com/demo/two.html doesn't show anything left behind, but it doesn't require a resize on drop I'll admit, other than from the insertBar.

HiroyasuNishiyama commented 4 years ago

@adumesny Sorry for the late response. The current implementation of dashboard layout editor uses the old internal API of GridStack, this seems to be the root cause. I have tried to make fixes to upgrade to 0.6.4.

adumesny commented 4 years ago

glad you got the 0.6.4 going... let me know if you need help later going to 1.x (or wait for 2.x which I'm still working on - jquery removal from all API/events and typescript rewrite).

Nice to see a download uptick in gridstack from 16k to 26k this past week!

dceejay commented 4 years ago

I'm sure we will, given the struggles so far upgrading with non-breaking changes... anything that really breaks things will trickier. And as for v2 - maybe not unless there is some great functionality we are missing ? as we are still using jquery so it's not a burden for us to have it there.