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

Deduplicate addons in lazy engines that are nested dependencies of the engine #595

Merged
merged 5 commits into from May 11, 2019

Conversation

Projects
None yet
10 participants
@2hu12
Copy link
Contributor

commented Sep 28, 2018

Try to fix remaining "Shared addon deeper than engine" problem of #356.

An example of this problem copied from #356 (comment)

  • project has dep on addon-derp
  • project has lazy engine engine-a
  • engine-a has engine addon-z
  • addon-z has addon-derp (addon-derp should not be duplicated)

For the example above, the addon-derp should not be duplicated, but without this PR it would be duplicated in the vendor.js of engine-a.

@rwjblue @trentmwillis @stefanpenner

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2018

@2hu12 very nice, I was hoping to tackle this soon. But I am happy you beat me to it. Let me look over it :)

Show resolved Hide resolved lib/engine-addon.js Outdated
@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

@2hu12 thanks for adding tests, will check over this !

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

@stefanpenner Thanks! Do you have any other comment for it?

@mydea

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Would also love to have this, I tried this branch and it fixed our remaining deduplication issues perfectly.

@villander

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

@2hu12 @stefanpenner we need an optimizing deduplication hook in Ember engines which has insight across all boundaries with all information pulled out of variables, and then an asset dep tree to make sure that we can successfully merge nodes when their node and their subtrees are identical.

another point: we need to figure out if we want to allow different versions of the same addon to coexist (via namespacing)

@rwjblue @dgeb

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@2hu12 @stefanpenner we need an optimizing deduplication hook in Ember CLI which has insight across all boundaries with all information pulled out of variables, and then an asset dep tree to make sure that we can successfully merge nodes when their node and their subtrees are identical.

I think the addon duplication is an issue when there is a lazyLoading option and for now only ember-engine has this option, is that right?

another point: we need to figure out if we want to allow different versions of the same addon to coexist (via namespacing)

I have a similar question for @rwjblue on discuss with the answer.

@villander

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

I think the addon duplication is an issue when there is a lazyLoading option and for now only ember-engine has this option, is that right?

sorry I edited my comment. We make sure that we can successfully merge nodes when their node and their subtrees are identical. It's was completed with your PR?

I have a similar question for @rwjblue on discuss with the answer.

@2hu12 ember-cli is different of ember-engines project, we can have two engines with the same addon only with different versions (via namespacing). I talked about it on last week with @dgeb. The use case is very different. Rob also said that he thinks not, we are still not sure. And as you know more than I dependencies on ember-engines is something even more obscure yet undocumented, it's something I wanted very much to see settled.

@2hu12 2hu12 force-pushed the 2hu12:master branch 2 times, most recently from d779114 to 34d1ee4 Jan 21, 2019

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

@2hu12 and myself are kicking the tires.

@tomdale

This comment has been minimized.

Copy link

commented Mar 14, 2019

@2hu12 Awesome work!

@mydea

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Fyi, I've also put this into an example repo where the issue could be seen before - and it nicely fixes the duplication: https://github.com/mydea/test-ember-engines-addons 👍 Would love to see this land!

@scalvert

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Super awesome work here, @2hu12! This has been a huge problem for us for a while. I can't wait to see this land.

@stefanpenner stefanpenner force-pushed the 2hu12:master branch from 34d1ee4 to d86afa2 Mar 27, 2019

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

@2hu12 I've rebased your PR.

Show resolved Hide resolved lib/utils/deeply-non-duplicated-addon.js
Show resolved Hide resolved lib/utils/deeply-non-duplicated-addon.js
Show resolved Hide resolved lib/utils/recover-addons.js Outdated
Show resolved Hide resolved lib/engine-addon.js Outdated
return;
}

dedupedAddon._orginalAddons = dedupedAddon.addons;

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Mar 27, 2019

Collaborator

Although this works in common situation, I believe it poses some broader risks. For example, if the patched addon instance is referred to in async or callback code, it will appear to have divergent addons arrays.

Although this scenario is rare, when it occurs it will be extremely surprising:

Contrived example to illustrate:

treeForSomething(tree) {
  let addonsWhenHookCalled = this.addons;
  return new BroccoliPlugin(tree, {
    someCallbackInvokedLater: () => {
      let addons = this.addons; // imagine we need the list of addons here for some reason

      addons !== addonsWhenHookCalled; // this becomes surprising and incoherent
     }
  });
}

I've thought about how to fix this, I believe it can be done by utilizing a proxy, which wrap's an add-on, and traps when addon.addons is accessed, providing only the addons that should be visible. (And doing so recursively).

Does taking this approach sound reasonable? Or do we will the existing approach is tolerable?

cc @rwjblue / @dgeb

This comment has been minimized.

Copy link
@2hu12

2hu12 Apr 1, 2019

Author Contributor

Could any treeForSomething in engine-addon.js or ember-lic addon have async behaviour? It's seems everything is sync like return addon[methodName].apply(addon, invokeArguments); or it should return a Promise.
Is there any real world example rather than a contrived one?

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Apr 1, 2019

Collaborator

@2hu12 contrived, and for us to discuss. If we don't feel it is worth it, we don't need to doit.

Show resolved Hide resolved lib/engine-addon.js Outdated

@2hu12 2hu12 force-pushed the 2hu12:master branch from d86afa2 to d15ce3f Apr 9, 2019

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Looks like CI failures are due to ember-cli-addon-tests always using emberjs/data#master instead of a released version (the emberjs/data repo was recently changed into a monorepo so linking directly this way is invalid). Trying to fix that over in tomdale/ember-cli-addon-tests#208...

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

@2hu12 2 more things, then we can merge + release ( just confirmed with @rwjblue, he is also +1)

  • readme addition re: this flag
  • CI green.
@villander
Copy link
Member

left a comment

thanks for the hard work @2hu12

@2hu12 2hu12 force-pushed the 2hu12:master branch from 7833fa7 to 74f1565 Apr 11, 2019

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

  • readme addition re: this flag

Let's wait for the CI green.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

ember-cli-addon-tests@0.11.1 is released, and likely fixes the CI failures around ember-data packages missing

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

#639 bumps the version independently, so we can confirm that CI is green on master...

@villander

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@rwjblue @stefanpenner CI continues broken

I think that we can merge this and generate v0.8.0 release, because we know that CI error happens because ember-cli-addon-tests and don't impact anything on codebase that do ember-engines to work ; )

@gabrielcsapo

This comment has been minimized.

Copy link

commented Apr 18, 2019

@villander so excited for this!

@2hu12 and everyone thanks for this!

@villander

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@stefanpenner @rwjblue any news here?

@scalvert

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

If we can get #639 merged, we can get CI back to green. Right now that PR is failing independently. I'll try to poke at it to see what's going on.

@villander

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

thanks @scalvert

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

It seems that the CI is failed against ember-source@beta and canary for some incompatibility brought by FEATURE ember-glimmer-angle-bracket-built-ins.

@buschtoens

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@2hu12 Correct, ember-beta and ember-canary are failing because of the <LinkTo> PR. I have a fix here: #642.

However, there is still another regression in Ember: emberjs/ember.js#17963

Don't know why yarn test:node is failing though.

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@buschtoens Very glad you already find a way to fix this problem, I have some changes based on your version buschtoens#23. Also, I have another PR emberjs/ember.js#17971 which try to fix the remaining failed test.

@scalvert

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

What's currently blocking this PR? Anything I can help with?

@villander

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@scalvert some guys are trying solve a issue com dynamic route properties on canary here #642 but without success.

@2hu12

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@scalvert, @buschtoens has a PR for fixing the failing test of ember@beta and ember@canary, but the ember@beta also has a bug on {{#link-to}} component which might be fixed emberjs/ember.js#17971, the fix still needs a test.

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

I am going to merge and release this, the test failures are unrelated to this PR and it provides good value as it is. The test failures are from sources outside of this repo, and are independently being moved forward.

I don't normally merge when red, but after careful consideration and review, and also that this has been open for so long, I am making an exception.

@stefanpenner stefanpenner merged commit f7666a8 into ember-engines:master May 11, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@villander

This comment has been minimized.

Copy link
Member

commented May 11, 2019

thanks @stefanpenner

@stefanpenner

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

released as v0.7.1 🎉thanks @2hu12 for both your patience, and pushing this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.