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

Critical dependency: the request of a dependency is an expression - /webgpu/webgpu-graphics-device.js #5730

Closed
AlbertoMalavasi opened this issue Oct 8, 2023 · 14 comments · Fixed by #5737
Assignees
Labels
area: graphics Graphics related issue bug

Comments

@AlbertoMalavasi
Copy link

AlbertoMalavasi commented Oct 8, 2023

Title: Critical dependency error in playcanvas module with webpack

Issue description

When building my project, webpack throws a critical dependency warning associated with the playcanvas module.

The warning states

WARNING in ./node_modules/playcanvas/build/playcanvas.mjs/platform/graphics/webgpu/webgpu-graphics-device.js
Critical dependency: the request of a dependency is an expression

Steps to Reproduce


import * as pc from 'playcanvas';

export default class ViewerManager {

    constructor() {
        console.log(pc)
    }
}

Environment

Node: ^16.0.0 || ^18.0.0
npm: ^7.0.0 || ^8.0.0 || ^9.0.0
playcanvas version: 1.66.0
webpack version: ^5.76.3

Expected behavior

The project should build without any critical dependency warnings.

Actual behavior

Webpack throws a warning regarding a critical dependency issue in the playcanvas module.

@kungfooman
Copy link
Collaborator

Thank you for pointing it out!

Happens since #5711

We should stick to document.createElement('script') in src/platform/graphics/webgpu/webgpu-graphics-device.js

In addition to this webpack issue, we can also no longer use relative paths because of import.

@LeXXik
Copy link
Contributor

LeXXik commented Oct 8, 2023

I believe it is due to those 2 lines:

const results = await Promise.all([
import(twgslUrl).then(module => twgsl(twgslUrl.replace('.js', '.wasm'))),
import(glslangUrl).then(module => module.default())
]);

Here the guys suggest to explicitly convert to string:
https://stackoverflow.com/questions/42908116/webpack-critical-dependency-the-request-of-a-dependency-is-an-expression

However, it sounds like a webpack issue? Not sure the engine should support it, as it has chosen rollup.

@kungfooman
Copy link
Collaborator

However, it sounds like a webpack issue? Not sure the engine should support it, as it has chosen rollup.

We have to be agnostic about that, the engine (ESM files in npm package) are consumed by third-party bundlers to create a user bundle

@mvaligursky
Copy link
Contributor

Any thoughts on this @slimbuck ?

@mvaligursky
Copy link
Contributor

We have a mentioned of it here: https://forum.playcanvas.com/t/engine-release-v1-66-0/33297/3

@slimbuck
Copy link
Member

The previous method of loading glslang with a script tag was actually loading a 'loader script' which performed the dynamic import (see here).

The loader script was downloading the actual wasm glue script via CDN (unacceptable) and using a dynamic import statement anyway (though it included the webpackIgnore comment and so worked with webpack).

So it seems we have two options to address this issue:

  • add /* webpackIgnore: true */ to the import statement and update tooling to keep the comment (by default comments are stripped)
  • use the string approach found by @LeXXik above

The second option is the simplest and least invasive.

Can anyone test that the following works correctly with webpack?:

        const results = await Promise.all([
            import(`${twgslUrl}`).then(module => twgsl(twgslUrl.replace('.js', '.wasm'))),
            import(`${glslangUrl}`).then(module => module.default())
        ]);

@kungfooman
Copy link
Collaborator

Can anyone test that the following works correctly with webpack?:

Just tested and it definitely works (red: as it is currently, green: with your fix):

image

@slimbuck
Copy link
Member

Lovely - thank you so much! I'll merge the fix then.

@pavle-goloskokovic
Copy link

The issue still happens if webpack tries to use prebuilt es5 playcanvas engine (node_modules/playcanvas/build/playcanvas.js) instead the default mjs.

One possible solution is to add noParse option to the module section of your webpack config:

...
module: {
        ...
        noParse: [
            // ignore imports and requires in playcanvas build
            /playcanvas\./
        ],
        ...
}
...

@kungfooman
Copy link
Collaborator

Wow, what a chain of issues rewriting that dynamically added <script> to import, thank you for reporting - it makes sense, since template strings are transpiled.

Can't you use ESM version aswell if you put {type: "module"} in your package.json? Transpiling down to ESM should be easy, while you can't "up-transpile" the ES5/UMD version to ESM.

@marklundin
Copy link
Member

It's likely this script import will be cause this issue as well.

@pavle-goloskokovic
Copy link

Can't you use ESM version aswell if you put {type: "module"} in your package.json? Transpiling down to ESM should be easy, while you can't "up-transpile" the ES5/UMD version to ESM.

@kungfooman I use Typescript so using default ESM import works, but my output target is ES5 and I import prebuilt ES5 version of the playcanvas engine to avoid needing to recompile entire playcanvas source code on each update during development and when creating production builds.

@Mistale
Copy link
Contributor

Mistale commented Feb 26, 2024

We're in the same boat as @pavle-goloskokovic . While we can use ESM in our projects, it would require us to turn on transpilation of dependencies in node_modules to ensure that we can still support ES5.

In our case, webpack tries to resolve the dynamic imports at build time and spits out hundreds of errors stemming from files within the ESM-module folder of the playcanvas package. The noParse trick works well, but we would prefer if the issue could be solved on the playcanvas side rather than requiring downstream projects to use special webpack configurations in order to get the commonjs version to work with webpack.

@mvaligursky
Copy link
Contributor

@marklundin is just cooking this one, any feedback?
#6090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants