PrismarineJS / prismarine-provider-anvil

Anvil Storage Provider implementation.
13 stars 24 forks source link

Require chunks impl lazily #70

Closed zardoy closed 10 months ago

zardoy commented 1 year ago

I'm working on loading needed data for prismarine web client on demand. With current impl all chunks are required which leads to require 1.13 mc data: https://github.com/PrismarineJS/prismarine-provider-anvil/blob/2d10dfef4a535e53d7b645132ced52f4b19cf5bb/src/1.13/chunk.js#L1-L2

rom1504 commented 1 year ago

isn't data needed by lot of other packages ?

zardoy commented 1 year ago

Good question. There are indeed some other packages that always require hardcoded version of mc-data at some point. overall I have discovered only 3 spots including this one. But I managed to easily workaround them so I can load needed mc data only when needed. By doing this I could reduce main bundle size to just 5mb. Overall its also a good pattern to execute only needed code I think.

zardoy commented 1 year ago

Hm, also why we also do require 1.13 since and not actual mc version? like this one:

 const ChunkSection = require('prismarine-chunk')(mcData.version.majorVersion).section 
rom1504 commented 1 year ago

We tried to put in common the code. 1.13 is the implementation for multiple versions

On Sun, Sep 3, 2023, 11:14 Vitaly @.***> wrote:

Hm, also why we also do require 1.13 since and not actual mc version? like this one:

const ChunkSection = require('prismarine-chunk')(mcData.version.majorVersion).section

— Reply to this email directly, view it on GitHub https://github.com/PrismarineJS/prismarine-provider-anvil/pull/70#issuecomment-1704060244, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR437WNPBWX7VMP76EARRDXYRC6FANCNFSM6AAAAAA4D3DKII . You are receiving this because you commented.Message ID: @.***>

zardoy commented 1 year ago

We tried to put in common the code. 1.13 is the implementation for multiple versions

So I guess it would not cause any problems if we use require('prismarine-chunk')(mcData.version.majorVersion).section?

rom1504 commented 1 year ago

where do you mean ?

rom1504 commented 1 year ago

feels like this is going to break webpack

are you sure it's not the case ?

extremeheat commented 1 year ago

A bundler will need to include all the JavaScript and JSON files statically anyway, so I don’t understand how this is supposed to work. The only difference I see would be saving some ram on nodejs by not loading unnecessary things from the file system

zardoy commented 1 year ago

A bundler will need to include all the JavaScript and JSON files statically anyway

Not always. Actually, I wanted to post it in prismaine-web-client later instead, but since you asked I'll do it here.

So I moved prismarine web client to esbuild completely (which is the same as webpack but less configurable, but faster). It has two configurations now:

  1. to prepare mc-data. Config is relative simple
  2. to bundle everything except mc-data

Whenever the client asks for a specific version (or with autoVersion, the version is guessed) it downloads mc-data of that version and then creates or joins to the server.

update: I'm not still not sure of this approach, will try to investigate using diffs between versions so there is no duplicating data.

where do you mean ?

https://github.com/PrismarineJS/prismarine-provider-anvil/blob/2d10dfef4a535e53d7b645132ced52f4b19cf5bb/src/1.13/chunk.js#L1-L7

move inside of the function

feels like this is going to break webpack

Only dynamic imports can break webpack require('prismarine-chunk') is not dynamic. Or did i get you wrong?

extremeheat commented 1 year ago

Whenever the client asks for a specific version (or with autoVersion, the version is guessed) it downloads mc-data of that version and then creates or joins to the server.

Downloading is not a synchronous process, it's asynchronous. There is no waiting here (the function wrapper makes no difference) so I don't understand.

zardoy commented 1 year ago

I'm talking about prismarine-web-client. The downloading of needed data happens before the server starts (or you connect to the server). However, when I try to load a 1.8.8 save it crashes, since the server, in this case, would require src/chunk.js with version 1.8, but it will also try to load mc-data of 1.13, 1.14, 1.15, 1.16, 1.17 as well. Btw in node, this is also a lot of mc-data to process! I have measured and in my case, it takes ~210ms to just initialize the chunkImplementations variable in this file when I start flying-squid. Coming back to the web version. Of course, I could workaround this issue by providing an empty dataset from mc-data, but this can lead to late errors like cannot read property blocks of undefined or something like that. I would like to catch these errors (when the wrong version is used) as soon as possible. And isn't it the right way: you require data of the needed version only? For me, this pattern has obvious benefits. Feel free to ask more questions.

rom1504 commented 10 months ago

merging

I will revert if anyone complains

rom1504 commented 10 months ago

/makerelease

zardoy commented 10 months ago

I will revert if anyone complains

not sure why anyone would :) but let me know in any case