search

embroider-build / ember-auto-import

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

Expected hash or Mixin instance, got [object Undefined] after upgrading to 2.6.0 #564

Closed oliverlj closed 1 year ago

oliverlj commented 1 year ago

I don't know where is the problem When I start I have this in the console with a white page

index.js:106 Uncaught Error: Assertion Failed: Expected hash or Mixin instance, got [object Undefined]
    at assert (index.js:106:13)
    at buildMixinsArray (mixin.js:509:13)
    at Mixin.reopen (mixin.js:435:38)
    at DebugFrameworkObject.extend (core.js:370:12)
    at Module.callback (core_view.js:13:40)
    at Module.exports (loader.js:106:1)
    at Module._reify (loader.js:143:1)
    at Module.reify (loader.js:130:1)
    at Module.exports (loader.js:104:1)
    at Module._reify (loader.js:143:1)

When I start the test, it says :

not ok 3 Chrome 109.0 - [undefined ms] - Global error: Uncaught Error: Could not find module `@ember/test-helpers` imported from `cryptofiscafacile-gui/tests/test-helper` at http://localhost:7357/assets/vendor.js, line 266
    ---
        browser log: |
            {"type":"error","text":"Uncaught Error: Could not find module `@ember/test-helpers` imported from `cryptofiscafacile-gui/tests/test-helper` at http://localhost:7357/assets/vendor.js, line 266\n","testContext":{"state":"complete"}}
jelhan commented 1 year ago

Several users of ember-bootstrap report seeing this error after upgrading to v2.6.0. I was able to trace it down to this line of code deep in Ember's internals:

define("@ember/-internals/views/lib/views/core_view", ["exports", "@ember/-internals/metal", "@ember/-internals/runtime", "@ember/object/evented", "@ember/object/-internals", "@ember/-internals/views/lib/views/states"], function (_exports, _metal, _runtime, _evented, _internals, _states) {
  "use strict";

  Object.defineProperty(_exports, "__esModule", {
    value: true
  });
  _exports.default = void 0;
  var __decorate = void 0 && (void 0).__decorate || function (decorators, target, key, desc) {
    var c = arguments.length,
      r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc,
      d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);else for (var i = decorators.length - 1; i >= 0; i--) {
      if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    }
    return c > 3 && r && Object.defineProperty(target, key, r), r;
  };
  class CoreView extends _internals.FrameworkObject.extend(_evented.default, _runtime.ActionHandler) {
    constructor() {
      super(...arguments);
      this.isView = true;
    }
   // ...
}

_runtime.ActionHandler is undefined. This causes the assertion to throw. It seems that @ember/-internals/metal resolves to an empty object.

Please find a reproduction here: https://github.com/jelhan/test-ember-bootstrap-with-ember-auto-import-v2-6-0 You will see the error when starting the application with yarn start.

Steps to reproduce if not using that repository:

  1. Create a new Ember application with Ember CLI 4.10
  2. Install Ember Bootstrap with ember install ember-bootstrap
  3. yarn start and open http://localhost:4200 in Browser

The assertion is logged to the console.

NullVoxPopuli commented 1 year ago

Do these problems exist with 2.5.0, or 2.4.0?

ef4 commented 1 year ago

@NullVoxPopuli you could use the reproduction shared above and see if the earlyBootSet feature you worked on addresses this case.

jelhan commented 1 year ago

This is a regression in v2.6.0. It works fine with earlier versions as far as I know.

NullVoxPopuli commented 1 year ago

I'll take a look at the repro

jelhan commented 1 year ago

I'll take a look at the repro

I created a PR demonstrating that it works fine with v2.5.0 here: https://github.com/jelhan/test-ember-bootstrap-with-ember-auto-import-v2-6-0/pull/1

Edit: I fixed the linting issue. CI on main using ember-auto-import@2.6.0 fails with the error mentioned in the title of this GitHub issue. CI passes for the PR downgrading to v2.5.0.

jelhan commented 1 year ago

@NullVoxPopuli asked on Discord to test if opting out of earlyBootSet introduced with v2.6.0 fixes the issue. It does: https://github.com/jelhan/test-ember-bootstrap-with-ember-auto-import-v2-6-0/pull/2 This indicates even more that it is a regression introduced by v2.5.0.

Ember Bootstrap has a npm download count in the top 10% of all addons accordingly to Ember Observer. The bug was reported by several people both on Discord as well as GitHub. If there isn't a quick fix, please consider rolling back the changes in v2.6.0 and releasing v2.5.0 again as v2.7.0 to mitigate the impact while debugging.

jelhan commented 1 year ago

I thought @ember/-internals/metal may just be missing in the default early boot set defined. https://github.com/ef4/ember-auto-import/blob/29eab602ff01d2c89b0140a0c2c0e534ad7d5b41/packages/ember-auto-import/ts/webpack.ts#L50-L57 I tried adding it in https://github.com/jelhan/test-ember-bootstrap-with-ember-auto-import-v2-6-0/pull/3 but it doesn't seem to fix the issue. Maybe because it is not a real package?

NullVoxPopuli commented 1 year ago

Hm, why is bootstrap importing from internal packages tho? That's not public api?

I mean, we can try adding that to the bootset, and maybe with a warning that this addon is using private api. Might be better to go that route i'll debug

NullVoxPopuli commented 1 year ago

According to the breakpoint at exception, we are trying to access this module:

@ember/-internals/views/lib/views/core_view
// which is imported from 
@ember/-internals/view/index
// which is imported from.... etc
@ember/-internals/glimmer/index
@ember/component/index
@glimmer/component/index
__v1-addons__early-boot-set__
rsvp
@ember/-internals/runtime/lib/ext/rsvp
@ember/-internals/runtime/index
@ember/application/index
test-ember-bootstrap-with-ember-auto-import-v2.6.0/app

So... this error is confusing, because these modules are all available to addons and apps.

So, tabling the possibility for a module loading issue for the time being, going back the error at the top of the stack: image Silly old CoreView... mixing in "FrameworkObject" with "Evented" and "ActionHandler"... mixins :upside_down_face:

but anywho, back to what the error is talking about... one of the mixins is undefined: image (the second one) which is passed in here: image so...

For background, what is earlyBootSet?

It's a feature to help with a load-order issue between the two module worlds, broccoli vs webpack. Specifically, when a v2 addon imports a v1 addon which is declared as a peerDependency -- provided by the app. This is concisely described by this test: https://github.com/ef4/ember-auto-import/blob/main/test-scenarios/v2-addon-test.ts#L468

And the investigation intially occurred in this issue: https://github.com/ef4/ember-auto-import/issues/504

At the time of merging, we only expected errors related to this problem to pop up when folks import modules from v2 addons that are from v1 addons (in general -- we don't yet have a way to solve #504, generically)

Those v1 addons need to be added to the earlyBootSet so that they are loaded before the v2 addon modules are loaded.

So, why is the runtime module missing?

Things I tried (because at this point, I need to look in other directions, and figure out what is causing the problem):

This seems... unrelated to the early boot set, yet maybe still similar. Adding rsvp to the early bootset does nothing because rsvp is not an ember addon

jelhan commented 1 year ago

Hm, why is bootstrap importing from internal packages tho? That's not public api?

Ember Bootstrap does not import from internal packages. The error is triggered deep in the internals of Ember. It is triggered by the mixing of this class: https://github.com/emberjs/ember.js/blob/e77595745a05d160c0efec996c45adb35e569b74/packages/%40ember/-internals/views/lib/views/core_view.ts#L27

The bug is very likely not specific to Ember Bootstrap. I assume it is triggered in other cases as well. At least I'm not aware of anything very specific to Ember Bootstrap in this regard. I assume it is just noticed and reported by users of Ember Bootstrap first due to high usage of that addon.

Could we please consider switching the default for earlyBootSet? Enabling it by default in a minor release caused a regression affecting a relevant portion of the ecosystem. Having it disabled by default and allowing early adopters to opt-in if needed seems to be the safer option.

NullVoxPopuli commented 1 year ago

The error is triggered deep in the internals of Ember.

Do you read my big comment? 😅

likely not specific to Ember Bootstrap.

In this case it occurs due to Bootstrap importing rsvp

Could we please consider switching the default for earlyBootSet?

It's too late for that now, and I'd like to know why rsvp is causing all this. The default earlyBootSet solves a ton of problems, and it's not 'just some feature', removing the defaults will break a ton of apps.

There is a work around for the time being, and that will unblocked all who are affected:

autoImport: {
  earlyBootSet: () => []
}

However, larger projects will have issues with this, as described in #504

if needed seems

It's needed until we get core packages to the v2 format, including ember-source.

ef4 commented 1 year ago

It is not too late. It’s entirely up to us whether to roll the feature back.

I let it ship only because I didn’t have time to do what I consider the correct solution instead (analyzing the webpack stats output to set the correct dependencies on every AMD stub we generate), and deferred to this one, but I still find it risky.

On Sun, Feb 12, 2023 at 7:48 AM NullVoxPopuli @.***> wrote:

The error is triggered deep in the internals of Ember.

Do you read my big comment? 😅

Could we please consider switching the default for earlyBootSet?

It's too late for that now, and I'd like to know why rsvp is causing all this. The default earlyBootSet solves a ton of problems, and it's not 'just some feature', removing the defaults will break a ton of apps.

if needed seems

It's needed until we get core packages to the v2 format, including ember-source.

— Reply to this email directly, view it on GitHub https://github.com/ef4/ember-auto-import/issues/564#issuecomment-1427024610, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACN6MVI6DZA7CBNVIZHDJDWXDLYHANCNFSM6AAAAAAULPL32I . You are receiving this because you commented.Message ID: @.***>

jelhan commented 1 year ago

removing the defaults will break a ton of apps

Do you have data backing that one? I haven't seen much bug reports for v2.5.0 and earlier versions related to this. It would be surprising if many apps started to rely on this default behavior in the last few weeks.

NullVoxPopuli commented 1 year ago

Have you read through #504 ? the issue is really hard to grok, and anyone running in to it will hold off on adopting v2 addons. When that particular issue comes up, folks may not even know what the issue is, because of how errors are reported.

For example, the class of issues resolved by the fix to #504 is most commonly presented as:

Issue #504 affected all ember-auto-import versions supporting v2 addons prior to 2.6.0. It has been a major blocker for mass adoption of v2 addons. I'm 100% certain I'm not the only one (I didn't report the original issue), but in a company that maintains >= 180 ember projects, we run in to stuff, and due to the scale, that stuff becomes blockers. Some people would give up, potentially thinking to themselves "oh, there is nothing I can do", "this is broken", "this looks like this other similarly reported error (maybe: https://github.com/ef4/ember-auto-import/issues/536)", and then not proceed with incremental v2 addon conversion across the ecosystem. (I have no idea how to extrapolate how many people I know don't report issues in to impact -- I know people run in to a lot of things and say nothing) Converting addons to the v2 format is essential for our long term success and performance :tada:

While I agree we need to fix the issue affecting RSVP / ember-bootstrap, I do not agree with removing the feature, especially considering the temporary work-around.

Could I personally deal with churn in this feature if we were to roll it back / make it opt in? absolutely. But I do not have any confidence that doing so would make things better for the whole of users, especially as more of the ecosystem converts to v2 addons. (I kinda think it would make things worse, as people are more likely to run in to #504 than when the problem was "fixed" :upside_down_face: )

ef4 commented 1 year ago

@NullVoxPopuli until someone produces a complete description of what's causing this issue, you have no way of knowing whether it's rare or common, or whether #533 actually fixed #504, or just fixed some cases and made others worse.

If you can't figure it out, and if I don't have time to do it for you soon, rolling back is the better thing for the community because an old bug that people were living with is better than destabilizing previously stable patterns with a new bug of unknown severity.

NullVoxPopuli commented 1 year ago

I will be digging deeper tomorrow

NullVoxPopuli commented 1 year ago

Except, this whole thing bothers me and I don't feel like I can relax :(

further investigation / things I've noticed:

Not really super helpful, because these are all things from within ember-source (aside from @glimmer/component)

But, searching for what the early-boot-set is applied to when @ember/routing/route is only added gives us a snapshot in to what the early boot set is applied to (without too much noise): image

so, this is probably why things are going haywire -- RSVP has a dependency on the early boot set, which includes v1 addons (ember-source during the error case), but ember-source depends on RSVP -- so we have a cycle

My plan now is to figure out a way to omit adding __v1-addons__early-boot-set__ as a dependency to the staticImports modules that would create a cycle if we the boot set were added.

  {{#each staticImports as |module|}}
    d('{{js-string-escape module.specifier}}', ['__v1-addons__early-boot-set__'], function() { return require('{{js-string-escape module.specifier}}'); });
  {{/each}}

this code presently adds the early boot set to all modules -- which we don't need to do -- because we need to ignore fake dependencies which are included in ember-source

this approach will still be flawed though, because I don't think we deeply analyze dependencies. thankfully, so far, all problems I've encountered with this feature (now and during its development) have only been around ember-source... and I presently think all this complexity goes away when ember-source is converted to a v2 addon

NullVoxPopuli commented 1 year ago
yeah, [this is "a" fix](https://github.com/NullVoxPopuli/test-ember-bootstrap-with-ember-auto-import-v2-6-0/blob/main/patches/ember-auto-import%2B2.6.0.patch) ```diff diff --git a/node_modules/ember-auto-import/js/webpack.js b/node_modules/ember-auto-import/js/webpack.js index 99bbc10..a0311f8 100644 --- a/node_modules/ember-auto-import/js/webpack.js +++ b/node_modules/ember-auto-import/js/webpack.js @@ -78,6 +78,10 @@ handlebars_1.registerHelper('js-string-escape', js_string_escape_1.default); handlebars_1.registerHelper('join', function (list, connector) { return list.join(connector); }); +handlebars_1.registerHelper('is-in-ember-source', function (modulePath) { + // TODO: how to determine if ember-source has stopped bundling these? + return ['rsvp', '@glimmer/component', '@glimmer/tracking'].some(dep => modulePath.startsWith(dep)); +}); const entryTemplate = handlebars_1.compile(` module.exports = (function(){ var d = _eai_d; @@ -95,7 +99,11 @@ module.exports = (function(){ }; d('__v1-addons__early-boot-set__', [{{{v1EmberDeps}}}], function() {}); {{#each staticImports as |module|}} - d('{{js-string-escape module.specifier}}', ['__v1-addons__early-boot-set__'], function() { return require('{{js-string-escape module.specifier}}'); }); + d('{{js-string-escape module.specifier}}', [ + {{#unless (is-in-ember-source module.specifier)}} + '__v1-addons__early-boot-set__' + {{/unless}} + ], function() { return require('{{js-string-escape module.specifier}}'); }); {{/each}} {{#each dynamicImports as |module|}} d('_eai_dyn_{{js-string-escape module.specifier}}', [], function() { return import('{{js-string-escape module.specifier}}'); }); ```

now that I have a path to explore and add an automated test for (thanks for the reproduction @jelhan !!), I can relax, and finish this tomorrow (for reals this time)

jelhan commented 1 year ago

@NullVoxPopuli @ef4 Any update on this one? We got this reported by additional users of Ember Bootstrap in this week. In case the bug fix needs a few more days or weeks, please consider rolling back or at least changing default value as interim solution.

NullVoxPopuli commented 1 year ago

I submitted a PR here: https://github.com/ef4/ember-auto-import/pull/566

Esaron commented 1 year ago

I believe this issue caused two severe app failures for my company in the past month and stumped most of us for a not-insignificant amount of time. First was an Ember upgrade that included the ember-auto-import dependency bump, then the dependency bump on its own (which we were holding off on due to the Ember upgrade being in flight). We independently narrowed down the issue to RSVP and pulled it out of a fork of ember-bootstrap to get things working, but an upstream fix would be preferable. We've since tried setting earlyBootSet to () => [] and that workaround does seem to allow the app to load for us as well.

When that particular issue comes up, folks may not even know what the issue is, because of how errors are reported.

The same can be said of this issue. We had no clue what to do or how to move forward due a very opaque error, and 2.5.0 wasn’t broken for us. It doesn't even always throw in the same way due to what I assume is the timing-related nature of the underlying issue. For the initial Ember upgrade, we saw Uncaught TypeError: Cannot set property default of #<Object> which has only a getter, while just the dependency threw the error Uncaught TypeError: Cannot read properties of undefined (reading 'extend'). Furthermore, this issue only impacted us when EMBER_ENV was production.

PS - Sorry about all the comment edits, just want to provide as much context as I can.

ef4 commented 1 year ago

Fixed by #568 in release 2.6.1.

jelhan commented 1 year ago

Can confirm that it is fixed. Haven't tested myself. But serveral users of Ember Bootstrap reports that it's working again. Thanks a lot.

simonihmig commented 1 year ago

I just spent a few hours on the opposite case. Nevertheless posting here, as a lot of the discussion around #504 was taking place here...

I have a reproduction at https://github.com/CrowdStrike/ember-headless-form/tree/eai-repro (run pnpm start and see /tests), where:

Given that, it is possible to fix this using either:

So it seems again we don't have a version that "just works"(TM)