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

transformer: define does not share reference for objects #7641

Closed
sapphi-red opened this issue Dec 4, 2024 · 6 comments
Closed

transformer: define does not share reference for objects #7641

sapphi-red opened this issue Dec 4, 2024 · 6 comments
Assignees
Labels
C-enhancement Category - New feature or request

Comments

@sapphi-red
Copy link
Contributor

sapphi-red commented Dec 4, 2024

esbuild try

#[test]
fn same_reference_for_object() {
    let config =
        ReplaceGlobalDefinesConfig::new(&[("__OBJ__", r#"{"vars":{"SOMEVAR":"foo"}}"#)])
            .unwrap();
    test(
        "
        console.log(__OBJ__.vars);
        __OBJ__.vars.ANOTHER = 'bar';
        console.log(__OBJ__.vars)",
        "
        const __OBJ__ = {vars:{SOMEVAR:\"foo\"}};
        console.log(__OBJ__.vars);
        __OBJ__.vars.ANOTHER = 'bar';
        console.log(__OBJ__.vars)",
        config.clone(),
    ); // fails
}

Oxc outputs:

console.log({ 'vars': { 'SOMEVAR': 'foo' } }.vars);
({ 'vars': { 'SOMEVAR': 'foo' } }).vars.ANOTHER = 'bar';
console.log({ 'vars': { 'SOMEVAR': 'foo' } }.vars);

This has a different behavior.

@sapphi-red
Copy link
Contributor Author

I'm not sure how many people relies on this behavior though

@IWANABETHATGUY
Copy link
Contributor

after investigation,

  • webpack DefinePlugin: don't share object
  • rollup: AFAIK they don't have the official define plugin(they have replace plugin based on string), don't support share object.
  • oxc define: don't share object
  • esbuild define: create a share object

So I am wondering if it is necessary to add this feature.

@sapphi-red
Copy link
Contributor Author

It seems the reason was to reduce the bundle size rather than sharing the reference itself. evanw/esbuild#581
I don't think it's a necessary thing, but I guess it'd be difficult to introduce this behavior later on as an optimization, as it changes the behavior.

@IWANABETHATGUY
Copy link
Contributor

I see

@Boshen
Copy link
Member

Boshen commented Dec 8, 2024

I want to reject this feature for now and evaluate the requirement when it is bought up again.

The motivation and discussion in evanw/esbuild#581 is still unclear to me, and the I can't find any real examples of people using this feature.

And also, the behaviour of esbuild documented in evanw/esbuild@a5288c6#diff-f312cd3ffe8d88cf424d3f0bd04b41e5b0ad36e549874b5688028bf9091a61f5R967 is

This release adds support for array and object literals with --define anyway, but they work differently than other --define expressions. In this case a separate virtual file is created and configured to be injected into all files similar to how the --inject feature works. This means there is only at most one copy of the value in a given output file. However, these values do not participate in constant folding and dead code elimination, since the object can now potentially be mutated at run-time.

Personally, this feature is really confusing in the scope of the define plugin, which makes it behave like an automated inject plugin.

Let's revisit this requirement again when it is bought up again by a consumer.

@Boshen Boshen changed the title define does not share reference for objects transformer: define does not share reference for objects Dec 8, 2024
@Boshen Boshen added C-enhancement Category - New feature or request and removed C-bug Category - Bug labels Dec 8, 2024
@Boshen
Copy link
Member

Boshen commented Dec 9, 2024

I'm going to close this as not planned for now, feel free to bring this up again when there is an actual requirement.

@Boshen Boshen closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants