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

Add DependenciesProvider #88

Closed
wants to merge 7 commits into from

Conversation

nataly87s
Copy link
Contributor

related to #87

client/core/lib/Client.ts Outdated Show resolved Hide resolved
@eladav
Copy link
Contributor

eladav commented Jan 19, 2020

nice change. I think that the most important thing here is changing the prefix for the overriding client's cache. I'll talk to @EladBezalel and we'll try to challenge this solution to see that everything works fine. In the meantime can you please check the the example project works the same? I think you did not break any API but just to be sure.

@nataly87s nataly87s force-pushed the add-dependencies branch 2 times, most recently from 3bfcbbf to d16cb23 Compare January 21, 2020 20:02
@nataly87s
Copy link
Contributor Author

@eladav example project works the same

@nataly87s nataly87s requested a review from eladav January 21, 2020 20:12
@nataly87s nataly87s force-pushed the add-dependencies branch 3 times, most recently from 6bdeb83 to 53aad8a Compare January 21, 2020 21:32
@nataly87s
Copy link
Contributor Author

@eladav @EladBezalel what's up? any comments? requests? something?

@eladav
Copy link
Contributor

eladav commented Jan 30, 2020

@nataly87s sorry for the delay...looking at both PRs now.

@eladav
Copy link
Contributor

eladav commented Jan 30, 2020

So the reason we're having a hard time with this change is more conceptual than code-wise. The implementation looks 💯.
It looks like you want to be able to show a dynamic component from within a dynamic component. Given that you're going to use work spaces it must be quite easy to just bundle the host component along with the displayed component instead of having another client and another host-id which might make it harder for you to realize which component is shown at what configuration.

If you worry about dependencies and bundle size it might still be easier to bundle the child component along with the host component as it already has the required dependencies.

Am I missing something? wdyt?

@EladBezalel feel free to chime in if you're available 🤔

@nataly87s
Copy link
Contributor Author

@eladav I want to have an app with as few dependencies as possible, and a dynamic component that will inject all the other dependecies, so that if I want to update a package I won't have to publish a new version of the app, just a new version of the main component.
The app will only have native dependencies along with react and dynamico.

@nataly87s
Copy link
Contributor Author

@eladav @EladBezalel sup?

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

Successfully merging this pull request may close these issues.

2 participants