Skip to content

Conversation

@severo
Copy link
Contributor

@severo severo commented Nov 18, 2024

I removed hightable dependency from utils: I think hightable should depend on @hyparam/utils instead.

See #46.

From

dist/index.es.min.js  925.19 kB │ gzip: 284.48 kB │ map: 2,073.04 kB
dist/index.umd.min.js  659.19 kB │ gzip: 256.25 kB │ map: 2,014.01 kB

To

dist/index.es.min.js  205.69 kB │ gzip: 115.30 kB │ map: 54.02 kB
dist/index.umd.min.js  202.76 kB │ gzip: 114.79 kB │ map: 53.21 kB

We still bundle hycompressors in the inlined worker, which explains >200kB

@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

I also changed in components:

From

dist/index.es.min.js  958.91 kB │ gzip: 293.95 kB │ map: 1,679.42 kB
dist/index.umd.min.js  684.28 kB │ gzip: 265.02 kB │ map: 1,620.09 kB

to

dist/index.es.min.js  237.65 kB │ gzip: 124.17 kB │ map: 317.31 kB
dist/index.umd.min.js  225.98 kB │ gzip: 122.78 kB │ map: 314.00 kB

@severo severo requested a review from platypii November 18, 2024 15:03
@severo severo marked this pull request as ready for review November 18, 2024 15:03
@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

Hyparquet demo:

From

dist/index.html                         1.18 kB │ gzip:   0.57 kB
dist/assets/huggingface-Df0G9hpA.svg    5.91 kB │ gzip:   2.63 kB
dist/assets/hyparquet-Cs3XnChP.mp3     24.62 kB
dist/assets/index-CHa8szko.css         10.88 kB │ gzip:   3.36 kB
dist/assets/index-D_yEg-np.js         509.54 kB │ gzip: 213.44 kB

to

dist/index.html                         1.18 kB │ gzip:   0.57 kB
dist/assets/huggingface-Df0G9hpA.svg    5.91 kB │ gzip:   2.63 kB
dist/assets/hyparquet-Cs3XnChP.mp3     24.62 kB
dist/assets/index-CHa8szko.css         10.88 kB │ gzip:   3.36 kB
dist/assets/index-rFP8huDy.js         367.46 kB │ gzip: 167.96 kB

@platypii
Copy link
Contributor

This is a great improvement in the bundle size!

But, I would really prefer not to have hightable to take dependency on @hyparam/utils since hightable should be usable independently, and I worked really hard to keep it dependency-free.

The problem is that we need some of the utils from hightable, mostly related to the DataFrame types and related utilities. And it seems like when we import from hightable, it's including more than just those utils? Are we not tree-shaking correctly? Or is it the fact that hightable has a peerDependency on react?

This PR seems good but I don't want to approve until we understand what we're going to do with hightable deps.

@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

OK! I was wondering the same... Maybe it's a matter of tree-shaking that I didn't manage to make work (since we don't use React dependent code from hightable).

I was also wondering at some point if hightable should be one of the @hyparam/components, as I see it as one of the main elements of the hyparam apps? But as per your comment above, I understand that you want to keep it independent, right?

@platypii
Copy link
Contributor

platypii commented Nov 18, 2024

At the moment I think that hyparquet and hightable are the two components which i would like to keep separate from hyparam components. I think they are useful enough to stand on their own, and hopefully even get some community participation (already happening with hyparquet). I think that is more likely to happen if they are standalone projects not deeply tied to hyperparam components.

Can you take a look at better tree-shaking? It would be nice to not have to duplicate rowCache. I have seen problems before when you import from the package directly then it loads the entire main js file (in this case dist/HighTable.min.js) and doesn't tree shake correctly because the react component is also "imported" even if it's not used. Some ideas:

  1. Change hyparam components to import more specific files from the hightable package: import { DataFrame } from 'hightable/src/DataFrame.ts'
  2. Change hightable to have an index.js which imports and re-exports, but does not contain the HighTable component inline.
  3. Specifically exclude react dependency from components in package.json? (ugly but might work?)

@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

Yes, looking at it!

@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

it was that simple! #48

@severo
Copy link
Contributor Author

severo commented Nov 18, 2024

superseded by #48

@severo severo closed this Nov 18, 2024
@severo severo deleted the exclude-react-from-packages branch November 18, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants