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

Bug on prune on MacOS & nodejs #1140

Closed
Makio64 opened this issue Oct 25, 2023 · 8 comments · Fixed by #1147
Closed

Bug on prune on MacOS & nodejs #1140

Makio64 opened this issue Oct 25, 2023 · 8 comments · Fixed by #1147
Labels
bug Something isn't working external Problems or limitations traced back to other tools. package:functions
Milestone

Comments

@Makio64
Copy link
Contributor

Makio64 commented Oct 25, 2023

Describe the bug
The following code is working well on Nodejs Windows but got errors on Nodejs MacOS.
It seems related to nodeTreeShake / treeShake function getting undefined when call in themself.

To Reproduce
Steps to reproduce the behavior:

this is all the commands i tested which fail( and worked as expected under nodejs windows )

// BUG1 Failed to get file info: ReferenceError: nodeTreeShake is not defined at prune
await document.transform(flatten())
await document.transform(weld({ tolerance: params.weldTolerance, toleranceNormal: params.weldToleranceNormal })) 
await document.transform(simplify({ simplifier: MeshoptSimplifier, ratio: params.simplifyRatio, error: params.simplifyError }))

//BUG2 Failed to get file info: ReferenceError: treeShake is not defined at prune
await document.transform(reorder({ encoder: MeshoptEncoder }), quantize({ pattern: /^(POSITION|TEXCOORD|JOINTS|WEIGHTS)(_\d+)?$/ })) 

Expected behavior
No error on both.

Versions:

  • Version: gltf-transform ^3.7.4"
  • Environment: nodejs v20.7.0 / macbook pro (MacOS Ventura 13.6)

Additional context
Compile under electron-vite
Windows & Mac use latest nodejs version

@Makio64 Makio64 added the bug Something isn't working label Oct 25, 2023
@donmccurdy
Copy link
Owner

Hmmm that's a surprise! Do you mind checking if you get the same error in Node.js v18? I develop on macOS, so I'm not sure what might be different here.

@Makio64
Copy link
Contributor Author

Makio64 commented Oct 25, 2023

Yeah I was also surprise.

I just tested and got the same errors with Nodejs v18.9.0

I'm using electron-vite latest, electron latest and this in my .jsconfig for the project :

{
  "compilerOptions": {
    "module": "esnext",
    "target": "esnext",
    "baseUrl": "./src/renderer",
      "paths": {
        "@/*": ["./src/*"]
      }
  },
  "include": ["src"]
}

also excepted prune() all the other commands seems to work fine

@donmccurdy donmccurdy added this to the v3.7 milestone Oct 25, 2023
@Makio64
Copy link
Contributor Author

Makio64 commented Oct 26, 2023

I rectify, I just switch back on windows this morning and same bug. I revert to older gltf-transform version
3.7.0 fine
3.7.1 fine
3.7.2 fine
3.7.3 fine ( both mac / windows )
3.7.4 broken ( same bug on both mac / windows )

@javagl
Copy link

javagl commented Oct 26, 2023

Same here (Windows 10, Node v18.12.1)

Before I saw this issue, I zoomed into that by creating an empty project with a package.json of

{
  "name": "gltf-transform-version-issue",
  "version": "0.0.0",
  "dependencies": {
    "@gltf-transform/core": "3.7.4",
    "@gltf-transform/functions": "3.7.4"
  },
  "devDependencies": {
    "ts-node": "^10.9.1",
    "typescript": "^4.8.3"
  }
}

and a VersionIssue.ts like

import { Document } from "@gltf-transform/core";
import { prune } from "@gltf-transform/functions";

async function run() {

  const document = new Document();
  const node = document.createNode();
  const scene = document.createScene();
  scene.addChild(node);
  await document.transform(prune());
}

run();

Doing
npm install
npx ts-node ./VersionIssue.ts
works fine when the version is pinned to 3.7.3, but with 3.7.4 it prints

C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\functions\src\prune.ts:107
                if (propertyTypes.has(PropertyType.NODE) && !options.keepLeaves) root.listScenes().forEach(nodeTreeShake);
                                                                                             ^
ReferenceError: nodeTreeShake is not defined
    at transform (C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\functions\src\prune.ts:107:94)
    at body (C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\core\src\document.ts:214:10)
    at t (C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\core\src\document.ts:211:15)
    at _forOf (C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\core\src\document.ts:231:5)
    at vt.transform (C:\glTF-Transform-Version-Issue\node_modules\@gltf-transform\core\src\document.ts:212:49)
    at run (C:\glTF-Transform-Version-Issue\VersionIssue.ts:10:18)
    at Object.<anonymous> (C:\glTF-Transform-Version-Issue\VersionIssue.ts:13:1)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module.m._compile (C:\glTF-Transform-Version-Issue\node_modules\ts-node\src\index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)

@donmccurdy
Copy link
Owner

I see, thanks @javagl and @Makio64! I'm still baffled by what's broken in 3.7.4, but I'll revert the changes for now and publish 3.7.5 soon, so at least the latest version will be stable.

@donmccurdy
Copy link
Owner

I've published v3.7.5 (duplicate of v3.7.3) for now. I will need to find some time to look into what's wrong on v3.7.4.

@donmccurdy
Copy link
Owner

Changes in #1127 hit a bug in the build system, breaking the prune() function only in the CommonJS builds.

Context:

Still trying to figure out if I can configure Microbundle to output async/await to CommonJS builds without transpiling, or what alternatives are available if not.

@donmccurdy donmccurdy added dependencies Pull requests and issues related to dependencies external Problems or limitations traced back to other tools. and removed needs investigation dependencies Pull requests and issues related to dependencies labels Oct 30, 2023
@donmccurdy
Copy link
Owner

I've refactored the prune() function to avoid the transpiler bug, and published a new release to v3.8. In v4 I hope to make some larger build changes that would skip this transpiler step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working external Problems or limitations traced back to other tools. package:functions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants