chore: remove rollup - Githubissues

lichess-org / chessground

Mobile/Web chess UI for lichess.org

https://lichess.org

GNU General Public License v3.0

1.02k stars 262 forks source link

chore: remove rollup #251

Closed LeoDog896 closed 1 year ago

LeoDog896 commented 1 year ago

Removes redundant rollup build. Now that the dist folder is freed, moves the main files over to dist.

Other changes are purely formatting changes made with npm run format.

niklasf commented 1 year ago

This bundle is currently used over at https://github.com/lichess-org/lila/blob/master/public/javascripts/vendor/chessground.min.js. We first need to update lila to produce the bundle in a different way, if we want to remove this here. Doing that instead of checking the compiled file into source control definitely makes sense, though.

LeoDog896 commented 1 year ago

Sounds good 👍, if you need any help on the lila side do let me know.

niklasf commented 1 year ago

@schlawg Any oppinion on this? I see that lichess-pgn-viewer also makes rollup bundles, but does not include them in the npm package.

LeoDog896 commented 1 year ago

I did some digging and it seems to be the way lila does it legacy. I'll make a refactor PR to bundle all necessary dependencies, which:

should decrease bundle size (and therefore increase performance)
open us to code splitting (woo! js perf!)
open upstream dependencies such as chessground and pgn-viewer to removal of the unnecessary rollup toolchain
simplify ui/@build logic (because of this fake bundling, there's convolution in the way the build scripts work. It makes sense if it was still the case, but if we handle all dependencies via a bundler, we can remove this logic too.)

I'll get started soon and link the PR here.

schlawg commented 1 year ago

I just now saw your note Niklas. I'm not familiar with chessground or this issue but I'm sure it's fine, let me know if you need anything LeoDog.

LeoDog896 commented 1 year ago

The issue is a much more deeply rooted problem in lila that comes down to how lila optimizes dependencies.

schlawg commented 1 year ago

I'm all for fixing deep rooted problems! Keep in mind that for any major changes you have in mind, you'll need to demonstrate concrete benefits that outweigh the churn cost. Keeping sync with a notion of best practices is commendable but not even close to sufficient.

LeoDog896 commented 1 year ago

I'll close this for now.