Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for dynamic imports in Vite apps so tiles render #5502

Merged
merged 2 commits into from
May 10, 2023

Conversation

aruniverse
Copy link
Member

There are long outstanding issues with Vite, see, where dynamic imports behave differently between development and production builds.

See our users reporting issues: #5185

@aruniverse aruniverse requested a review from johnnyd710 May 10, 2023 17:02
@aruniverse aruniverse requested review from kabentley, bbastings and a team as code owners May 10, 2023 17:02
@pmconne
Copy link
Member

pmconne commented May 10, 2023

Something special about this dynamic import that makes it different from our various other dynamic imports, or are they also ticking time bombs?

core/backend/src/rpc/web/response.ts:91:  const { Readable, Stream } = await import(/* webpackIgnore: true */ "stream");
core/backend/src/rpc/web/response.ts:92:  const { createGzip } = await import(/* webpackIgnore: true */ "zlib");
core/frontend/src/ApproximateTerrainHeights.ts:42:      const { terrainHeightsPropsString } = await import("./ApproximateTerrainHeightsProps");
core/frontend/src/gltf/GltfParser.ts:325:      const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/extension/providers/ExtensionLoadScript.ts:17:  // const module = await import(/* webpackIgnore: true */jsUrl);
core/frontend/src/extension/providers/ExtensionLoadScript.ts:20:  // Because tsc transpiles "await import" to "require" (when compiled to is CommonJS).
core/frontend/src/tile/TileAdmin.ts:323:      await import("reflect-metadata");
core/frontend/src/tile/TileAdmin.ts:325:      const { AzureFrontendStorage, FrontendBlockBlobClientWrapperFactory } = await import(/* webpackChunkName: "object-storage" */ "@itwin/object-storage-azure/lib/frontend");
core/frontend/src/tile/GltfReader.ts:1465:      const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/tile/PntsReader.ts:147:    const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;

@pmconne
Copy link
Member

pmconne commented May 10, 2023

Have we checked why Vite behaves this way and if it's expected?

@aruniverse
Copy link
Member Author

Something special about this dynamic import that makes it different from our various other dynamic imports, or are they also ticking time bombs?

This pkg in particular, itwin/object-storage-azure, is a cjs pkg; and this seems to be an issue in vite only when dynamically importing cjs, see issue

core/backend/src/rpc/web/response.ts:91: const { Readable, Stream } = await import(/* webpackIgnore: true / "stream");
core/backend/src/rpc/web/response.ts:92: const { createGzip } = await import(/
webpackIgnore: true */ "zlib");

AFAIK, no one is using Vite to bundle their backends, so we should be ok with these

core/frontend/src/ApproximateTerrainHeights.ts:42: const { terrainHeightsPropsString } = await import("./ApproximateTerrainHeightsProps");

This should be ok, as we deliver esm ver of core-frontend

core/frontend/src/gltf/GltfParser.ts:325: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/tile/GltfReader.ts:1465: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/tile/PntsReader.ts:147: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;

image

loaders.gl/draco delivers esm, so this should be safe. @johnnyd710 is testing this to verify

core/frontend/src/extension/providers/ExtensionLoadScript.ts:17: // const module = await import(/* webpackIgnore: true */jsUrl);
core/frontend/src/extension/providers/ExtensionLoadScript.ts:20: // Because tsc transpiles "await import" to "require" (when compiled to is CommonJS).

These are comments, and the ExtensionAPI should be able to handle cjs modules (which i imagine would still be rare)

core/frontend/src/tile/TileAdmin.ts:323: await import("reflect-metadata");

Should be ok, used for the side-effect

@pmconne
Copy link
Member

pmconne commented May 10, 2023

itwin/object-storage-azure, is a cjs pkg

Know of any plans to deliver esm? @paulius-valiunas @austeja-bentley

@johnnyd710
Copy link
Contributor

core/frontend/src/gltf/GltfParser.ts:325: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/tile/GltfReader.ts:1465: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;
core/frontend/src/tile/PntsReader.ts:147: const dracoLoader = (await import("@loaders.gl/draco")).DracoLoader;

image loaders.gl/draco delivers esm, so this should be safe. @johnnyd710 is testing this to verify

Confirmed, I can use readGltfGraphics to load a .glb into an app built with vite without any problems.

image

@aruniverse
Copy link
Member Author

itwin/object-storage-azure, is a cjs pkg

Know of any plans to deliver esm? @paulius-valiunas @austeja-bentley

This will be tricky, this pkg delivers both backend and frontend apis. I would like to rethink this pkg during the 4.x generation

@aruniverse aruniverse merged commit ed2c824 into master May 10, 2023
@aruniverse aruniverse deleted the fix/tiles-on-vite branch May 10, 2023 18:31
@austeja-bentley
Copy link
Contributor

itwin/object-storage-azure, is a cjs pkg

Know of any plans to deliver esm? @paulius-valiunas @austeja-bentley

This will be tricky, this pkg delivers both backend and frontend apis. I would like to rethink this pkg during the 4.x generation

We have not thought about delivering ESM modules yet. If there is a need for this, let's have a call sometime to discuss the details and time frames.

@paulius-valiunas
Copy link
Contributor

We have not thought about delivering ESM modules yet. If there is a need for this, let's have a call sometime to discuss the details and time frames.

Yes, I believe this is very much needed... CJS is obsolete, we even wanted to move iTwin.js Core to ESM in 4.0 but didn't have enough time to fix our tests. And because ESM is async, it would be easier if all our dependencies already supported ESM (including object-storage).

@kabentley
Copy link
Contributor

[I realize this PR has already merged, so i'm hoping this question will be found].

How did this problem affect iTS? As far as I understand, the current design is that desktop apps will only display tiles they generate, not ones loaded from cloud storage. What is the long term plan for that? I ask because i think we need a different plan for cloud storage for tiles than using an account key. @pmconne @wgoehrig

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.

6 participants