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

ember 2.13.0-beta.1 is broken (using the glimmer 2 VM v0.22) #48

Closed
toranb opened this issue Mar 23, 2017 · 26 comments
Closed

ember 2.13.0-beta.1 is broken (using the glimmer 2 VM v0.22) #48

toranb opened this issue Mar 23, 2017 · 26 comments

Comments

@toranb
Copy link
Collaborator

toranb commented Mar 23, 2017

I opened an issue w/ the ember project to see if they can help us nail down exactly how we should be clearing cache now in ember 2.13X

emberjs/ember.js#15057

The example app to show this broken can be found here for anyone interested.

I tried a few additional steps tonight (like clearing the _compilerCache and _templateCache) but no such luck. I'm hoping the ember core team can shed some light on the issue so we can continue to support hot reloading in the ember ecosystem :)

@toranb
Copy link
Collaborator Author

toranb commented May 14, 2017

Reminding myself this is still a big issue - broken in the current stable ember build 2.13.0

@MiguelMadero
Copy link
Collaborator

Quick update, @jcollins1991 and I were pairing on this. It seems like in 2.13 we already have a ComponentManager, that has a reference to the old ComponentClass. We didn't get it working all the way, but we were able to hack it from the debugger to make it use the correct class.

After that, I had another quick look and was able to get it working with a small patch to ember's source. I would like to find the right extension point. We're pairing again on this next week to try to find the right hook.

@jcollins1991 you can have a look at emberjs/ember.js#15057 for a few more details. Please check emberjs/ember.js#15057 (comment) for a rough idea of what a HotReplacementComponentManager may look like once emberjs/rfcs#213 is implemented.

@MiguelMadero
Copy link
Collaborator

MiguelMadero commented May 20, 2017

One more update, looks like the CurlyComponentManager is hardcoded at the moment when creating the glimmer environment (https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/environment.js)
One quick option is to provide our own GlimmerEnvironment, just so we can pass our own ComponentManager. I think that we could get that working next time. Alternatively, we could just send a PR to ember to be able to get the CurlyComponentDefinition factory from the resolver. This is likely a long route and may not even get implemented since this area will likely change a lot as the RFC gets implemented.

cc: @rwjblue @chancancode

@rwjblue
Copy link
Member

rwjblue commented May 20, 2017

emberjs/ember.js#15254 is the first steps to exposing custom managers behind a flag. We will likely make the curly manager resolve in that PR (so both custom and "default" managers follow the same path).

/cc @twokul

@toranb
Copy link
Collaborator Author

toranb commented May 20, 2017

@MiguelMadero @jcollins1991 @rwjblue you guys rawk :)

@twokul
Copy link

twokul commented May 20, 2017

@toranb @MiguelMadero I'm addressing comments from @rwjblue so we can hopefully land this today/tomorrow

@MiguelMadero
Copy link
Collaborator

That's awesome @twokul I won't have time this weekend, but I would like to try this sometime next week before even if it's directly from your branch.

@rwjblue
Copy link
Member

rwjblue commented May 20, 2017

@mamadero / @toranb / anyone else that groks how this works - Lets setup a meeting to discuss. I am not 100% certain that all of the use cases here will be addressed by custom component managers, so I want to make sure I understand the constraints in this addon and can better represent these concerns as we develop more and more glimmer infrastructure...

@MiguelMadero
Copy link
Collaborator

@rwjblue sure. We can set something up next week (mon-wed), Im OOO starting thurs for 2 weeks. I can show you what we debugged yesterday which could give you a good idea of what is needed. However, it may be more productive if we get something working on top of @twokul's branch to make things more concrete.

Btw, I have a different handle on GH (@MiguelMadero)

@twokul
Copy link

twokul commented May 20, 2017

@MiguelMadero @toranb @rwjblue I'd like to be a part of this discussion if possible

@MiguelMadero
Copy link
Collaborator

MiguelMadero commented May 21, 2017 via email

@toranb
Copy link
Collaborator Author

toranb commented Aug 13, 2017

@rwjblue any chance we could chat again about landing a PR to invalidate the template cache in ember > 2.12? This is what we are doing today and it's in part what broke as of ember 2.13+

function clear (context, owner, name) {
  var environment = owner.lookup('service:-glimmer-environment');
  if (environment) { // Glimmer2
    environment._definitionCache.store.clear();
  }
  if (owner.__container__) {
    clearIfHasProperty(owner.__container__.cache, name);
    clearIfHasProperty(owner.__container__.factoryCache, name);
    clearIfHasProperty(owner.__registry__._resolveCache, name);
    clearIfHasProperty(owner.__registry__._failCache, name);

    clearIfHasProperty(owner.base.__container__.cache, name);
    clearIfHasProperty(owner.base.__container__.factoryCache, name);
    clearIfHasProperty(owner.base.__registry__._resolveCache, name);
    clearIfHasProperty(owner.base.__registry__._failCache, name);
  } else {
    clearIfHasProperty(context.container.cache, name);
    clearIfHasProperty(context.container.factoryCache, name);
    clearIfHasProperty(context.container._registry._resolveCache, name);
    clearIfHasProperty(context.container._registry._failCache, name);
    // NOTE: the app's __container__ is the same as context.container. Not needed:
    // clearIfHasProperty(window.Dummy.__container__.cache, name);
    // clearIfHasProperty(window.Dummy.__container__.factoryCache, name);
    // NOTE: the app's registry, is different than container._registry. We may need this
    // clearIfHasProperty(window.Dummy.registry._resolveCache, name);
    // clearIfHasProperty(window.Dummy.registry._failCache, name);
  }
}

@MiguelMadero
Copy link
Collaborator

I have been a bit busy with other projects, but I'm still looking forward to wrapping this up.

@toranb
Copy link
Collaborator Author

toranb commented Aug 21, 2017

@MiguelMadero do you have any detailed notes from our previous meeting w/ @rwjblue that detail out the path forward for breaking cache?

@quantuminformation
Copy link

Really looking fwd to trying this.

@Samsinite
Copy link

Samsinite commented Oct 16, 2017

@toranb @rwjblue I'm interested in helping move this along. Is there are any pointers or suggestions that could be made to update this to work in the latest ember/ember-cli/glimmer?

@toranb
Copy link
Collaborator Author

toranb commented Oct 17, 2017

@Samsinite I'm honestly not sure myself. If you are curious and want to hack I'd start by taking a look at the factoryManagerCache in the container

You should know that the core team would prefer a RFC on the subject (not a hero like you/ or me hacking to make this addon work). I'm fine with either myself - just didn't want to get your hopes up that someone from core would get involved to revive this project

https://github.com/emberjs/ember.js/blob/master/packages/container/lib/container.js#L33

@MiguelMadero
Copy link
Collaborator

MiguelMadero commented Oct 17, 2017 via email

@Samsinite
Copy link

Thanks for the feedback guys. Does anyone feel like this has been explored enough as an addon to start to write an RFC that they feel could be accepted into core? I think it would be fun to just hack against at the moment honestly.

@MiguelMadero
Copy link
Collaborator

IMO, this has been explored enough, but my opinion shouldn't stop you from trying. It would certainly help you gain better understanding about what to write on that RFC if you later decide to go down that route.

@MiguelMadero
Copy link
Collaborator

Unfortunately, at this point I don't have time to own either, but I'm happy to help someone carry this forward.

@alexhancock
Copy link
Collaborator

I looked into this a bit today. I gave this a try and it seems to make the addon work again with a vanilla ember cli project using 2.17.0.

I admit I do not understand everything about the underlying reasons a change like this works, but I do know factoryCache is not a thing any more after this.

Without a broader discussion about the future of the Ember component manager interface, it will be hard to come to a great solution where this addon will be able to avoid making changes which rely on private APIs to continue working.

Let me know if you want me to open a PR with this change to get things working again in the meantime, and then also if anyone wants to continue that broader discussion so we can come up with a good public API solution for addons like this.

@toranb
Copy link
Collaborator Author

toranb commented Dec 11, 2017

@alexhancock I’d be happy to pull this in if it solves the ember 2.17 issue. A further private api debate has been prevanet but I’d gladly fix this for all those people stuck atm

@toranb
Copy link
Collaborator Author

toranb commented Dec 11, 2017

For anyone lurking ;) @alexhancock legit solved this for ember 2.13+

I'll get some feedback on the PR #52 and see if we can't release something before the end of the week :)

Thanks again Alex!!!

@toranb
Copy link
Collaborator Author

toranb commented Dec 13, 2017

I just published v0.1.9 and this is no longer an issue

@toranb toranb closed this as completed Dec 13, 2017
@alexhancock
Copy link
Collaborator

🎉 Thanks for integrating that change!

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

7 participants