6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://github.com/6pac/SlickGrid/wiki
MIT License
1.82k stars 424 forks source link

fix for hidden column issue fixes #981 #985

Closed 6pac closed 7 months ago

6pac commented 7 months ago

Testing fix - no action needed yet

6pac commented 7 months ago

@ghiscoding sorry to ask again, but I have updated the ts - what is the easiest way to get it compiled to the dist folder for testing? Either on GiHub so I can hit the example URL, or locally.

Also, I think this will fix all of the problems. You might want to test it.

ghiscoding commented 7 months ago

You'll have to update your PR though because you overwrote some of my code since your branch is slightly out of sync with master.

For the build, you can run either Dev mode or run a Prod Build with TypeScript Types

npm run dev           # without TS Types
npm run build:prod    # also builds TS Types

just running in Dev mode is typically enough to test, just don't include any of the files from the dist/.. folder in the PR, the dist folder should only be updated when releasing a new version.

I'll test it tonight after work, I thought it would you a few days so I already released a version during the weekend, we can push a patch version later though

6pac commented 7 months ago

Thanks. Yeah I noticed the leakage. A bit weird since I ran a pull, created the branch and the PR all within about 10 minutes.

ghiscoding commented 7 months ago

@6pac I just tried it and it's still not correct as far as I can see, the "Finish" column header still shows up when hiding it brave_xApCwpshYD

6pac commented 7 months ago

@ghiscoding it's a bit embarrassing, but I really can't work the TS tooling for local testing.

I tested in js so I know the patch works. It's just getting the ts to flow through to dist/browser that is causing problems. I tried the dev and prod builds (had to update nodejs to get rimraf to work), and it's changing the dist files but not updating them with the ts changes.

For example, the grid init is failing with

Uncaught TypeError: grid.getPubSubService is not a function

and getPubSubService is in the ts file but not any js.

I notice that there is a .js file in the src folder for each .ts file. It this .js file used or is it just an artefact of tsc? I'm not running tsc manually, I'm expecting the dev or prod build to do that.

What am I missing?

ghiscoding commented 7 months ago

that's weird, rimraf is supposed to delete the dist folder you could try to do that manually before starting because technically the process is to simply run npm run dev and use the example. However very important, when you start dev it should open a new browser window and you need to run the examples from there. You cannot test the example like before, so you cannot double click an example from file explorer to run them, you will not get latest code doing that way. You really have to run it from npm run dev, so you'll get a link similar to this for basic example http://localhost:8080/examples/example1-simple.html

Also make sure that you ran npm install, I updated the dependencies a few times (almost every time I do a release).

I notice that there is a .js file in the src folder for each .ts file. It this .js file used or is it just an artefact of tsc? I'm not running tsc manually, I'm expecting the dev or prod build to do that.

so inside the dist/browser folder that is normal and the expected behavior to get a transpiled JS file for each TS file from src, it is that way so that long time user of SlickGrid loading through <script> will have similar approach as before and load any plugins they way by loading whichever plugin JS file.

On the other hand, we have the CJS and ESM folders, those are single output file because new tools like RollupJS/Vite/WebPack will take care of the tree shaking, that process is executed when the user does a Prod build, it will only keep only the code you imported and remove anything else that is not associated to the import. For example if I do import { SlickColumnPicker, SlickGrid } from 'slickgrid', then the Prod build will only include code related to these 2, anything else will be removed from the final Prod build JS file.

In conclusion, the dist/browser cannot do that process because when you load through <script>, you are loading the entire content of that file, there is no tree shaking process involved which is why we need to keep separate JS files so that user can choose which JS file to load without loading everything else (you will never have a user that loads everything or at least very rarely, so for most users you want to keep their download size small)

you should get something like below in your folders

dist folder

image

src folder

image

ghiscoding commented 7 months ago

So I tried it again and I still see the same problem as before

6pac commented 7 months ago

OK, yes have got it working, I don't know what was going on before. There was some locking of npm packages that I had to reboot to get rid of, I think maybe npm was having a moment.
I also had .js files in my src folder, that must have been from when I was first checking out tsc. that was confusing and I have deleted them all.

Anyway, thanks for the advice and try again now. There was a single line of code missing.

I am aware of the purpose of the cjs and esm folders, although I don't meddle with them. Again thanks for the rundown.

ghiscoding commented 7 months ago

ah yeah that last missing line does seem to fix the issue for good :)

ghiscoding commented 7 months ago

@6pac you can probably give a try with the new structure and push a new release, just run npm run release. The process is the same as before, the only difference is that I added a new OTB (One Time Password) question, just type ENTER when that question comes since I think you're not using OTP yourself.

I think we're pretty much done with fixes and everything, so pushing a new release would be a good time I think. Cheers

6pac commented 7 months ago

@ghiscoding I'm happy for you to push releases (I think you've done the last few?). I do it so infrequently that it's likely I'll have tooling problems or forget to do something. Can do it if you really want, but I think it'd be better if I just did them if there was a need for me to do a number in a row.

ghiscoding commented 7 months ago

sure I can do it, I thought you maybe wanted to practice the releases 😋

6pac commented 7 months ago

Nope ;-)