Closed austinwoon closed 4 months ago
styfle
commented
5 months ago Looks like we're hitting this NODE_GYP_BUILD case correctly, however the AST is not understood and so node-gyp-build is never executed.
https://github.com/vercel/nft/blob/c16e7945c3f3221616567356d9abc2e824bf38cd/src/analyze.ts#L755
austinwoon
commented
5 months ago Looks like we're hitting this
NODE_GYP_BUILDcase correctly, however the AST is not understood and sonode-gyp-buildis never executed.https://github.com/vercel/nft/blob/c16e7945c3f3221616567356d9abc2e824bf38cd/src/analyze.ts#L755
Yes, I described why this was the case in the issue here
Datadog is using a CJS import and assigning it to a variable. So in the AST, we will not be able to infer the package name from the arguments (the arguments are findBinding, .., .. respectively, instead of node-gyp-build, .. or node-gyp-build, .., .. like in the other packages this statement was made for)
I also find that having to manually resolve the arguments via checking the array of args a little hard-codey, so i decided to just have a special case catch all instead for the package to just include prebuilds as it seemed simpler. Hope that makes sense!
austinwoon
commented
5 months ago Looks like we're hitting this
NODE_GYP_BUILDcase correctly, however the AST is not understood and sonode-gyp-buildis never executed.https://github.com/vercel/nft/blob/c16e7945c3f3221616567356d9abc2e824bf38cd/src/analyze.ts#L755
Could I also check if it would be possible to include a flag to ignore tree shaking modules in a package? I think it would make sense as a catch-all to handle special case like these!
austinwoon
commented
5 months ago I think we can revisit the general solution in a new PR and merge this PR once the windows test pass
I have removed the sourcemaps outputs from output.js since there is no actual need to test for those folders. Could you please rerun the test pipeline? Thank you
styfle
commented
4 months ago I took a stab at fixing this properly in PR https://github.com/vercel/nft/pull/419
Closes #410
Alternative solution considered
NODE_GYP_BUILDto account for problem identified in #410. This proved to be tricky as we need to retrieve the value fromfindBindingswhich involves a walk of the whole treeSolution
Instead of the alternative above, chose to just add a special case for this package
Automated tests
Just added a conditional
testName === '@datadog-pprof'to thefoundMatchingBinarystatement so we test the presence of theprebuildsfolder.Manual tests (since the automated tests do not test for presence of
prebuildsfolderRunning
node out/cli.js print node_modules/@datadog/pprof/out/src/index.jsgives me the following FILELISTWe see that the entire
prebuildsfolder is correctly included.