node-red-dashboard icon indicating copy to clipboard operation
node-red-dashboard copied to clipboard

update to latest gridstack.js

Open adumesny opened this issue 4 years ago • 44 comments

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.

adumesny avatar Mar 04 '20 17:03 adumesny

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

dceejay avatar Mar 04 '20 21:03 dceejay

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

adumesny avatar Mar 04 '20 22:03 adumesny

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 avatar Mar 04 '20 22:03 dceejay

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

dceejay avatar Mar 04 '20 23:03 dceejay

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.

adumesny avatar Mar 05 '20 00:03 adumesny

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.

dceejay avatar Mar 05 '20 00:03 dceejay

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.

adumesny avatar Mar 05 '20 00:03 adumesny

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,

dceejay avatar Mar 05 '20 00:03 dceejay

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 avatar Mar 05 '20 00:03 adumesny

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.

adumesny avatar Mar 05 '20 01:03 adumesny

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.

dceejay avatar Mar 05 '20 10:03 dceejay

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.

adumesny avatar Mar 05 '20 15:03 adumesny

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 ?

dceejay avatar Mar 05 '20 16:03 dceejay

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 ?

adumesny avatar Mar 06 '20 06:03 adumesny

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 :-)

dceejay avatar Mar 06 '20 08:03 dceejay

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

adumesny avatar Mar 06 '20 16:03 adumesny

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 ?

dceejay avatar Mar 06 '20 16:03 dceejay

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 avatar Mar 06 '20 17:03 adumesny

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

adumesny avatar Mar 06 '20 17:03 adumesny

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.

dceejay avatar Mar 06 '20 17:03 dceejay

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.

knolleary avatar Mar 06 '20 17:03 knolleary

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)

adumesny avatar Mar 06 '20 18:03 adumesny

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

dceejay avatar Mar 06 '20 18:03 dceejay

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

knolleary avatar Mar 06 '20 18:03 knolleary

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.

dceejay avatar Mar 06 '20 18:03 dceejay

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

adumesny avatar Mar 06 '20 19:03 adumesny

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

dceejay avatar Mar 06 '20 19:03 dceejay

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

adumesny avatar Mar 06 '20 19:03 adumesny

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 avatar Mar 06 '20 20:03 dceejay

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

dceejay avatar Mar 06 '20 22:03 dceejay