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

minification/bundling breaks library native is not defined. #66

Open
IanSSenne opened this issue Apr 22, 2022 · 9 comments
Open

minification/bundling breaks library native is not defined. #66

IanSSenne opened this issue Apr 22, 2022 · 9 comments

Comments

@IanSSenne
Copy link

I've build a small app using vite and when I run it in development it works fine but running it in production fails on load due to native not being defined.
vite: https://vitejs.dev/
I have attached a small project that replicates this issue.
vite-prismarine-nbt-bug1.zip

@IanSSenne IanSSenne changed the title minification breaks library native is not defined. minification/library breaks library native is not defined. Apr 22, 2022
@IanSSenne IanSSenne changed the title minification/library breaks library native is not defined. minification/bundlingbreaks library native is not defined. Apr 22, 2022
@IanSSenne IanSSenne changed the title minification/bundlingbreaks library native is not defined. minification/bundling breaks library native is not defined. Apr 22, 2022
@IanSSenne
Copy link
Author

one solution for this might be using new Function instead of eval and passing whatever is suppose to be native as a parameter.

@IanSSenne
Copy link
Author

IanSSenne commented Apr 22, 2022

I was able to fix this by changing pnbt.js (generated using the command in the readme) by updating Compiler.compile to the following

compile(code) {
                // Local variable to provide some context to eval()
                const native = this.native; // eslint-disable-line
                const { PartialReadError } = require("./utils"); // eslint-disable-line
                return new Function(
                  "native",
                  "PartialReadError",
                  "return (" + code + ")"
                )(native, PartialReadError)(); // eslint-disable-line
              }
              ```
even though this fixes it for me it may be worth looking into a longer term solution on the packages side.

@superx101
Copy link

superx101 commented Apr 19, 2024

I'm having the same problem with rollup.
I try solve it with the configuration:

export default [
    {
        // ... other
        output: {
            preserveModules: true
        },
        treeshake: false
    }
]

@LeoCaprile
Copy link

I'm having the same problem with rollup. I try solve it with the configuration:

export default [
    {
        // ... other
        output: {
            preserveModules: true
        },
        treeshake: false
    }
]

this config solved your problem? I tried but with no success

@extremeheat
Copy link
Member

node-protodef is compiling JS and running that with eval(), so the assumption is that the local variables are being captured by the eval(). Not fully sure what's going on here but it could be that perhaps the native var declaration is being deleted (as there are no references inside the codeblock) or it's being renamed somewhere.

Maybe doing something like a blank

Object.getOwnPropertyDescriptors({ native })

statement could fix it

@LeoCaprile
Copy link

node-protodef is compiling JS and running that with eval(), so the assumption is that the local variables are being captured by the eval(). Not fully sure what's going on here but it could be that perhaps the native var declaration is being deleted (as there are no references inside the codeblock) or it's being renamed somewhere.

Maybe doing something like a blank

Object.getOwnPropertyDescriptors({ native })

statement could fix it

Can I open a PR adding this fix?

@superx101
Copy link

superx101 commented May 8, 2024

I'm having the same problem with rollup. I try solve it with the configuration:

export default [
    {
        // ... other
        output: {
            preserveModules: true
        },
        treeshake: false
    }
]

this config solved your problem? I tried but with no success

yes, this config did solved my problem

the problem I located looks like this:

foo(expression) {
     const native = this.native;
     eval(expression); // native is used here
}

“native” is removed by rollup's default behavior

@extremeheat
Copy link
Member

Can I open a PR adding this fix?

You can open PR if you've tested it to work. The tree shaking functionality is in a way expected to cause problems because it's supposed to remove unused code. If this is the only place that there are problems I think it's ok to add a dummy statement (maybe even just Object(native) to handle this.

@IanSSenne
Copy link
Author

IanSSenne commented May 12, 2024

Can I open a PR adding this fix?

You can open PR if you've tested it to work. The tree shaking functionality is in a way expected to cause problems because it's supposed to remove unused code. If this is the only place that there are problems I think it's ok to add a dummy statement (maybe even just Object(native) to handle this.

I don't think this is sufficient due to minification being able to change the variable name, I think the best option is to wrap the code into a function instead of eval so the values can be named as arguments, the downside of this is some work will have to be done to make sure the code generated does not depend on things other than native as they will have to be specifically included in that scope

This library should really not require any specific configuration for bundling, keeping the variable from DCE does fix it for that config maybe but it won't help on other configurations where minification is involved

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

No branches or pull requests

4 participants