embroider-build / ember-auto-import

Zero config import from npm packages
Other
360 stars 109 forks source link

[2.7.3] side-effect imports are dropped #628

Closed runspired closed 5 months ago

runspired commented 5 months ago

I have a v2-addon with an entry-point designed to be a side-effect import. The module has export {} because some tools will DCE it otherwise, but overall its just a pure side-effect.

In the test app of a classic ember-cli app build using latest ember-auto-import and ember-cli I am importing the module in a test file to ensure it correctly performs its side-effect. It is not, because as it turns out the import of this side-effect module is dropped during build.

If I change the code to import * as foo from 'side-effect-module'; console.log(foo); the module import will not be dropped.

NullVoxPopuli commented 5 months ago

I could not replicate locally -- side-effecting imports work as expected in a fresh app.

(@runspired and I are pairing on this though)

NullVoxPopuli commented 5 months ago

current hypothesis is that something has gone very wrong with pnpm + dep management since the lockfile reveals that it is ignoring overrides 🙈

(auto-import and embroider/shared-internals (etc) were not up to date)

NullVoxPopuli commented 5 months ago

Gonna close for now. We can re open if there is a smell of auto-import (2.7.3+) issue later

runspired commented 5 months ago

this branch has the issue: https://github.com/emberjs/data/pull/9479 I've worked around for now by having the test assign the fake export from the side-effect to "this"

runspired commented 5 months ago

also I'm not convinced this is a pnpm issue where we ended up with the wrong auto-import, yes the wrong version is present some places in the project but we didn't narrow down to figuring out if the wrong one was present in the specific test app

ef4 commented 5 months ago

I'm running the reproduction and I see the same behavior. But I can see that ember-auto-import did include the module. It's the import statement in the test module that has been deleted.

If you comment out this line, the bug goes away and the side-effectful import remains.

So I think the problem is somewhere in your custom babel plugins.

runspired commented 5 months ago

@ef4 those plugins have already run before this ever occurs, and by commenting them out, all you've done is prevented auto-import and macros from running correctly at all.

runspired commented 5 months ago

I can't reopen this but its definitely not resolved and definitely not the fault of the babel plugins

ef4 commented 5 months ago

You could have given me the slightest benefit of the doubt and debugged those babel plugins to confirm, but OK, I did it for you. Here is where they are deleting side-effectful imports:

https://github.com/emberjs/data/blob/bb493919857cd86004c303fccbcdfe47852f8eef/packages/build-config/cjs-src/transforms/babel-plugin-transform-logging.js#L58-L60

ef4 commented 5 months ago

Most likely that code was intended to be guarded by the if statement above it, but it is not. So it deletes all side-effectful imports.

jelhan commented 3 months ago

I'm seeing a regression in Ember Bootstrap as well when upgrading from 2.7.2 to 2.7.4. Tests start to fail because sinon does not seem to be imported correctly anymore. Please see here for the PR upgrading the dependency: https://github.com/ember-bootstrap/ember-bootstrap/pull/2132

I haven't had time to debug in detail yet. Just wanted to mention that there might be a regression in 2.7.3 which affects more use cases.

NullVoxPopuli commented 3 months ago

Where is said import?

jelhan commented 3 months ago

Where is said import?

Let's look at one example.

This test is failing after the ugprade:

not ok 404 Chrome 126.0 - [31 ms] - Integration | Component | bs-modal/footer: Footer has close button
    ---
        actual: >
            null
        stack: >
            TypeError: (0 , _sinon.stub) is not a function
                at Object.<anonymous> (http://localhost:7357/assets/tests.js:9845:55)
        message: >
            Promise rejected during "Footer has close button": (0 , _sinon.stub) is not a function
        negative: >
            false
        browser log: |
    ...

This is where stub function is being used in that test: https://github.com/ember-bootstrap/ember-bootstrap/blob/6ec8e1fbc5ca200e1214e6970f148163e8baf96c/test-app/tests/integration/components/bs-modal/footer-test.js#L15

This is where stub function is imported from sinon: https://github.com/ember-bootstrap/ember-bootstrap/blob/6ec8e1fbc5ca200e1214e6970f148163e8baf96c/test-app/tests/integration/components/bs-modal/footer-test.js#L8

sinon is a dev dependency of the test app: https://github.com/ember-bootstrap/ember-bootstrap/blob/6ec8e1fbc5ca200e1214e6970f148163e8baf96c/test-app/package.json#L99

NullVoxPopuli commented 3 months ago

Are you sure it has a stub export? I've only ever seen folks use import sinon from 'sinon' and then use sinon.stub()

If you're using this file from sinon, that could be the case: image

jelhan commented 3 months ago

I think sinon supports that named export if consumed as an ECMAScript module.

Sinon declares in its package.json at line 118:

{
  "module": "./pkg/sinon-esm.js"
}

And in that huge file I can find at line 20295:

export { _stub as stub };

All of that refers to sinon version 18.0.0 published on NPM.

NullVoxPopuli commented 3 months ago

Sure, but do we know that the module export is used, even?

jelhan commented 3 months ago

Sure, but do we know that the module export is used, even?

I'm not an export on this topic. But from my debugging it seems that sinon is used as ECMAScript module with ember-auto-import@2.7.2 but as legacy module with ember-auto-import@2.7.3. With ember-auto-import@2.7.2 I see ./pkg/sinon-esm.js being used when inspecting the code at run-time. With ember-auto-import@2.7.3 it seems to use ./lib/sinon.js, which is the value of main and browser properties in sinon's package.json.

I noticed that Ember Bootstrap uses Sinon v17.0.1. But that does not make a difference regarding module, main, and browser entry points in its package.json.

It seems that ember-auto-import@2.7.3 uses a different entry point for the module than with previous versions.

NullVoxPopuli commented 3 months ago

excellent! thanks for poking a little further -- can you open a new issue for this? <3 thanks!!

ef4 commented 3 months ago

Please open a new issue for this discussion. It's not related to this original issue.