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

Glimmer VM v0.22 broke hot reloading #15057

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

Glimmer VM v0.22 broke hot reloading #15057

toranb opened this issue Mar 23, 2017 · 8 comments

Comments

@toranb
Copy link
Contributor

toranb commented Mar 23, 2017

I tested ember-cli-hot-loader today with ember v2.13.0-beta.1 and found that we are no longer able to clear the template/or compiler cache like we could previously with ember 2.12

A quick rundown of how the hot loader works => we clear all the cache we can find, then invoke rerender. Nothing magical but because ember 2.13.0-beta.1 takes a new version of the Glimmer 2 VM I'm wondering if you can help us identify how we should be clearing this cache now. I tried a few additional steps tonight, including what you see in the screenshot below (clearing not only _definitionCache, but also _compilerCache and _templateCache) but still no luck. I did step through the code to confirm it is executing as I found it was in ember 2.12 so my first guess is ... something has changed in glimmer 2 that we aren't yet clearing :)

screen shot 2017-03-22 at 8 56 44 pm

A full example app to show this broken can be found here. Any help would be amaz! Thank you in advance!

@rwjblue
Copy link
Member

rwjblue commented Mar 23, 2017

TBH, I don't really think this is an ember bug. The hot reloader is definitely digging around in some deeply private territory. I'm happy that folks enjoy using it, but at the moment it is definitely not using public APIs.

I think a good long term solution would be to truly analyze the need here, and submit an RFC to make the minimal required API that tools like this could hook into. That would give hot reloader (and other tools) the ability to stop using private API's...

@toranb
Copy link
Contributor Author

toranb commented Mar 23, 2017

@rwjblue understood - thanks for the quick reply!

@toranb toranb closed this as completed Mar 23, 2017
@rwjblue
Copy link
Member

rwjblue commented Mar 23, 2017

To be clear, my reply wasn't trying to "shut you down" or anything. As usually I'm happy to help try to find a path forward with you...

@toranb
Copy link
Contributor Author

toranb commented Mar 23, 2017

@rwjblue not to worry - my plan is to pause for the moment (as we have a big event next week). I really have 2 choices worth considering

  1. reboot and do the RFC you mention
  2. play hero and reverse engineer the internals with each glimmer VM change

@MiguelMadero and I have had great success to this point and I feel on some level the RFC is giving up. No offense to your comment ^^above asking me to play by the rules but this has been a powerful tool (for me personally) and I'd prefer to keep it moving (instead of rebooting /asking for feedback/ getting everyone involved when truly that addon was built for me)

@MiguelMadero
Copy link
Contributor

Hi,

tl;dr; let's extend this RFC or write a new one.

While I find the play hero approach fun, I also think that this could have a great wide impact if we could keep it stable for more than one Ember release. Additionally, even pre-glimmer 2, there are many known issues with the current hot-loader approach based on the component helper. Those issues are fixed with the AST approach, but we have not been able to release the new approach since it doesn't work with any Glimmer 2 version. I think it's about time to bite the bullet and do the less exciting work of writing that RFC.

I was talking with @chadhietala yesterday about this at the SF Ember Meetup and he said he could help me push this during the event next week and also recommended to look into an existing RFC that talks about the ComponentManager, I think we was referring to emberjs/rfcs#213. I've not had a chance to review it, but that might be an "official" way to solve this.

Going forward, I think we need help from someone with Glimmer 2 experience to 1) "play hero" together one more time to get a working version for the AST approach and 2) write the RFC so we can keep this stable going forward.

@MiguelMadero
Copy link
Contributor

MiguelMadero commented Mar 23, 2017

Apologies, the following is pretty rough, just an initial brain-dump. I'll polish it a bit and send a comment to the RFC to move that discussion there.

I had a first pass through emberjs/rfcs#213 and it seems promising. My initial thoughts are:

hot-replacement-component may be replaced by a hot-replacement-component-manager that looks something like:

class HotReplacementComponentManager extends ComponentManager<Object> {

  private components = new ComponentCache();

  create({ metadata }: ComponentDefinition, args: ComponentArguments) {
    let instance = getOwner(this).lookup(`component:${metadata.class}`);
    let otherValues = {path,otherValuesFromASTRewrite};
    return {instance, otherValues, metadata};
  }

  getContext({ instance }: Object): Ember.Component {
    return instance;
  }

  getView(createdItem: Object): Ember.Component {
    if (createdItem.otherValues.needsRefresh) {  // TODO: some how get this value from the service
      clearCache(createdItem.metadata.class); // Still need to clear require and container caches
      // magic happens here: 
      createdItem.instance = getOwner(this).lookup(`component:${createdItem.metadata.class}`);
    }
    return createdItem.instance;
  }

  update({ instance }: Object, args: ComponentArguments): void {
    instance.setProperties(args.named);
    instance.didUpdateAttrs();
    instance.didReceiveAttrs();
    // Plus other glimmer hooks, maybe simply call super here instead
  }
}

I'm making a lot of assumptions in our favor, but seems like the right direction.

@toranb
Copy link
Contributor Author

toranb commented Mar 23, 2017

@MiguelMadero first up - I wanted to apologize for speaking on your behalf yesterday. I respect your decision (leaning toward the RFC) because the total cost of ownership has grown out of control the past few months.

I'm conflicted because if someone has been using this addon with great success for some time and suddenly it's not supported /broken that reflects poorly on me (just something I'm dealing with here). I still plan to look deeper into the current break around mid April when I have the bandwidth but I certainly see value in moving ahead with the RFC as you suggested.

The truth (if people don't already know it?) is that you've done all the work anyway. My contribution was mostly in morale (to keep the dream alive/ help land something ember-cli might use itself one day). If you are headed one direction it's wise for me to consider the same as you are the brains behind that addon :)

@MiguelMadero
Copy link
Contributor

I wanted to apologize for speaking on your behalf yesterday.

@toranb all good! I didn't feel like you were.

is that you've done all the work anyway

That's kind of you to say that, but the reality is that we just unlocked different parts of the puzzle.

.... it's not supported /broken that reflects poorly on me...

I agree that's why I think we need to do both in parallel, fix what's broken and start discussing those public APIs that will be required to keep this stable. Hopefully we can get some help from someone with more Glimmer 2 experience, since to me that has been the main blocker for both.

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

3 participants