feat: expose application config as separate context #13

amcgee · 2019-06-11T15:29:48Z

This change does three things:

Add a config service which is the parent context of all other services, providing application configuration values such as base url and app name (to be expanded). Expose the new hook useAppConfig allowing access to this config information
consolidate all runtime providers (currently just ConfigProvider and DataProvider) into a single exported Provider component which includes everything needed by a DHIS2 app
Fix/update some of the top-level scripts and service/runtime build configs. Also update examples/cra to use hooks instead of consumer components.

I'm leaving DataProvider in as a top-level export to prevent a breaking change at this stage, it seems like we don't need it just yet and I want to be prudent about what we'd want to include in a 2.0 release.

amcgee · 2019-06-12T12:05:34Z

More eyes and thoughts on the api welcome.

HendrikThePendric

I think we should rethink the CurriedProviders component. I think the main purpose of having this component, is to prevent (potentially very deep) nesting of Providers. But IMO the current implementation comes with too many downsides. Loosing the Provider names is my biggest objection.

runtime/src/utils/CurriedProviders.js

HendrikThePendric · 2019-06-17T12:35:10Z

runtime/src/utils/CurriedProviders.js

+import React from 'react'
+import PropTypes from 'prop-types'
+
+export const CurriedProviders = ({ providers, children }) =>


Since this was implemented as a React component, this shows up as a component in React DevTools. We could possibly consider just implementing this as a function to prevent it happening. Or perhaps change the component name, so it reflects the purpose of this component rather than its technical implementation....

Since each provider is actually exported by its respective service package with the name Provider, we might need to manually override the displayName if we want them to be more informative in DevTools

Yes that is a good suggestion. Just tried setting a displayName: it works fine, but TypeScript doesn't like it.

However, since we are already doing this

import { Provider as ConfigProvider } from '@dhis2/app-service-config' import { Provider as DataProvider } from '@dhis2/app-service-data'

and additionally, we'd have to set a displayName....

Wouldn't it make more sense to just use a descriptive prefix in the exported variable name?

Or are there compelling reasons to keep the naming identical in every service?

@HendrikThePendric I like this idea. I went ahead and renamed all the exports from service packages (both Data and Context) to be more explicit, so they show up nicely in the React tree

See below for discussion of top-level provider name

runtime/src/utils/CurriedProviders.js

services/config/src/Provider.tsx

amcgee · 2019-06-18T08:06:20Z

@HendrikThePendric thanks for the review!

I think I agree that CurriedProviders could be improved, it's probably a premature optimization. Maybe it's best to just deeply nest the providers for now? It will only be done once (in the exported Provider) and isn't super deep yet.

amcgee · 2019-06-18T08:06:31Z

Accidentally closed sorry!

HendrikThePendric · 2019-06-18T08:33:31Z

Maybe it's best to just deeply nest the providers for now?

Yes I would be in favour of that.

amcgee · 2019-07-09T15:44:06Z

package.json

@@ -9,17 +9,20 @@
        "services/*"
    ],
    "devDependencies": {
-        "@dhis2/cli-style": "^3.0.1",
+        "@dhis2/cli-style": "~3.0.0",


NOTE I had to change this to a ~ range (so <3.1.0) because eslint was introduced in cli-style@3.1.0 and doesn't play nice with Typescript. It would be nice to support Typescript with our ESLint config

Working on supporting external ESLint plugins in: dhis2/cli-style#72

You can upgrade to 4.x of cli-style and get support for extended ESLint configs.

amcgee · 2019-07-09T15:47:59Z

@HendrikThePendric at long last I have now explicitly nested providers and gotten rid of the problematic <CurriedProviders> component

amcgee · 2019-07-09T16:09:22Z

Question @varl @HendrikThePendric et al -

What should the top-level "runtime provider" be called?

I originally had it called just Provider (import { Provider } from '@dhis2/app-runtime') but this isn't very descriptive when viewed in React dev tools (see screenshot above). I've changed it now to DHIS2RuntimeProvider, but maybe that's too verbose? Just RuntimeProvider?

varl · 2019-07-10T06:51:13Z

I've changed it now to DHIS2RuntimeProvider, but maybe that's too verbose? Just RuntimeProvider?

I prefer RuntimeProvider of those options.

Another alternative is ServicesProvider; maybe a combo, RuntimeServicesProvider.

Either way we can drop DHIS2.

edit: my vote goes to RuntimeProvider.

varl · 2019-07-10T07:01:00Z

This looks nice and clear now, introducing a breaking change at this point to clean up the namespace is better to do sooner than later.

Though I wonder if we will eventually end up in a place where re-export all providers individually so they can be tree-shaken out? that will never work when we deeply nest them, nor if we dynamically generate the provider tree at runtime. Only way to avoid the overhead would be if the app declares its own provider tree with the ones that it needs.

Hm, sounds like a future problem. 🤔 Defer.

amcgee · 2019-07-10T09:26:17Z

I've changed it now to DHIS2RuntimeProvider, but maybe that's too verbose? Just RuntimeProvider?

I prefer RuntimeProvider of those options.

Another alternative is ServicesProvider; maybe a combo, RuntimeServicesProvider.

Either way we can drop DHIS2.

edit: my vote goes to RuntimeProvider.

@varl yeah I like RuntimeProvider too. Normally I'm also against duplicate namespacing, but in this case maybe it's worth it to prepend DHIS2 so that it's obvious what this thing is in the React tree? Otherwise RuntimeProvider could come from literally anywhere.

We could possibly declare the component with the namespace (DHIS2RuntimeProvider or D2RuntimeProvider) and export it as just RuntimeProvider so that usage is import { RuntimeProvider } from '@dhis2/app-runtime' but it shows up in dev tools with the namespace? That could also be confusing though

Not sure I'm sold one way or the other, just some more thoughts

amcgee · 2019-07-10T09:28:39Z

Though I wonder if we will eventually end up in a place where re-export all providers individually so they can be tree-shaken out? that will never work when we deeply nest them, nor if we dynamically generate the provider tree at runtime. Only way to avoid the overhead would be if the app declares its own provider tree with the ones that it needs.

In AppShell world all of these providers are "provided" by the shell, so an App should actually never need them. If apps start breaking out of the Shell and becoming standalone again then maybe separate exports with treeshaking make sense, but I still think they'd want all of them. In any case if we're flopping back to fully independent apps we've got other problems ;-)

varl · 2019-07-10T10:09:49Z

@varl yeah I like RuntimeProvider too. Normally I'm also against duplicate namespacing, but in this case maybe it's worth it to prepend DHIS2 so that it's obvious what this thing is in the React tree? Otherwise RuntimeProvider could come from literally anywhere.

That's a valid concern to me, DHIS2 is more explicit than even D2. There can be no question about where that comes from.

amcgee · 2019-07-10T13:04:13Z

That's a valid concern to me, DHIS2 is more explicit than even D2. There can be no question about where that comes from.

So DHIS2RuntimeProvider @varl? If so thoughts on the "simplified" import usage mentioned above?

varl · 2019-07-10T14:04:47Z

So DHIS2RuntimeProvider @varl? If so thoughts on the "simplified" import usage mentioned above?

Just as a data point: there are currently 34 hits on all of GitHub in JavaScript for "RuntimeProvider": https://github.com/search?l=JavaScript&q=RuntimeProvider&type=Code

Is the simplified import really necessary? If the name bugs someone they could always do:

import { DHIS2RuntimeProvider as Provider } from '@dhis2/app-runtime`

Then again on the flip-side, if they have multiple RuntimeProvider's they could also do the reverse:

import { RuntimeProvider as DHIS2RuntimeProvider } from '@dhis2/app-runtime`

And we could have a shorter name in our library. 🤔

I prefer being consistent though and going for one or the other.

… into feat/app-context

amcgee · 2019-08-08T10:04:46Z

Following a Slack discussion last month, I've set the exported (in code) name to simply Provider but set the displayName of the component to DHIS2RuntimeProvider, which can also change at a later time. This gives informative react-dev-tool output while keeping the API clean

amcgee · 2019-08-08T11:32:40Z

I re-exported DataProvider (but not ConfigProvider), so this is no longer a breaking change and can still be used as-is outside the App Shell. We can make that break later if we want.

# [1.4.0](v1.3.0...v1.4.0) (2019-08-08) ### Features * expose application config as separate context ([#13](#13)) ([c9aa123](c9aa123))

dhis2-bot · 2019-08-08T11:47:46Z

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: expose application config as separate context

d1a43d7

amcgee requested a review from varl June 12, 2019 12:05

chore: remove unnecessary @babel/preset-react option

24b6a55

HendrikThePendric reviewed Jun 17, 2019

View reviewed changes

amcgee closed this Jun 18, 2019

amcgee reopened this Jun 18, 2019

amcgee mentioned this pull request Jul 5, 2019

fix(headerbar): links and app icons dhis2/ui-widgets#20

Merged

Merge branch 'master' into feat/app-context

f9599db

amcgee commented Jul 9, 2019

View reviewed changes

chore: explicitly nest providers

99d0fc1

chore: rename sub-package exports

3a18279

varl approved these changes Jul 10, 2019

View reviewed changes

amcgee marked this pull request as ready for review July 10, 2019 09:28

amcgee added 4 commits July 18, 2019 16:57

chore: upgrade cli-style, use typscript-eslint, fix eslint errors

d238122

Merge branch 'master' into feat/app-context

ae5dbce

chore: use generic Provider name in code, set displayName

32c3c69

Merge branch 'feat/app-context' of https://github.com/dhis2/app-runtime…

a9b2d67

… into feat/app-context

Merge branch 'master' into feat/app-context

616017c

amcgee changed the title ~~feat: BREAKING CHANGE expose application config as separate context~~ feat: expose application config as separate context Aug 8, 2019

amcgee merged commit c9aa123 into master Aug 8, 2019

amcgee deleted the feat/app-context branch August 8, 2019 11:36

amcgee mentioned this pull request Aug 8, 2019

Update @dhis2/cli-style to the latest version 🚀 #24

Closed

dhis2-bot pushed a commit that referenced this pull request Aug 8, 2019

chore(release): cut 1.4.0 [skip ci]

3598aec

# [1.4.0](v1.3.0...v1.4.0) (2019-08-08) ### Features * expose application config as separate context ([#13](#13)) ([c9aa123](c9aa123))

dhis2-bot added the released label Aug 8, 2019

This was referenced Aug 8, 2019

fix: don't break app-runtime API, export DataProvider as well #33

Merged

fix: support independent data provider, refactor example #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose application config as separate context #13

feat: expose application config as separate context #13

amcgee commented Jun 11, 2019 •

edited

Loading

amcgee commented Jun 12, 2019

HendrikThePendric left a comment

HendrikThePendric Jun 17, 2019

amcgee Jun 18, 2019

HendrikThePendric Jun 18, 2019

amcgee Jul 9, 2019

amcgee commented Jun 18, 2019

amcgee commented Jun 18, 2019

HendrikThePendric commented Jun 18, 2019

amcgee Jul 9, 2019

varl Jul 10, 2019

varl Jul 18, 2019

amcgee commented Jul 9, 2019

amcgee commented Jul 9, 2019 •

edited

Loading

varl commented Jul 10, 2019 •

edited

Loading

varl commented Jul 10, 2019

amcgee commented Jul 10, 2019

amcgee commented Jul 10, 2019

varl commented Jul 10, 2019

amcgee commented Jul 10, 2019 •

edited

Loading

varl commented Jul 10, 2019

amcgee commented Aug 8, 2019

amcgee commented Aug 8, 2019 •

edited

Loading

dhis2-bot commented Aug 8, 2019

feat: expose application config as separate context #13

feat: expose application config as separate context #13

Conversation

amcgee commented Jun 11, 2019 • edited Loading

amcgee commented Jun 12, 2019

HendrikThePendric left a comment

Choose a reason for hiding this comment

HendrikThePendric Jun 17, 2019

Choose a reason for hiding this comment

amcgee Jun 18, 2019

Choose a reason for hiding this comment

HendrikThePendric Jun 18, 2019

Choose a reason for hiding this comment

amcgee Jul 9, 2019

Choose a reason for hiding this comment

amcgee commented Jun 18, 2019

amcgee commented Jun 18, 2019

HendrikThePendric commented Jun 18, 2019

amcgee Jul 9, 2019

Choose a reason for hiding this comment

varl Jul 10, 2019

Choose a reason for hiding this comment

varl Jul 18, 2019

Choose a reason for hiding this comment

amcgee commented Jul 9, 2019

amcgee commented Jul 9, 2019 • edited Loading

varl commented Jul 10, 2019 • edited Loading

varl commented Jul 10, 2019

amcgee commented Jul 10, 2019

amcgee commented Jul 10, 2019

varl commented Jul 10, 2019

amcgee commented Jul 10, 2019 • edited Loading

varl commented Jul 10, 2019

amcgee commented Aug 8, 2019

amcgee commented Aug 8, 2019 • edited Loading

dhis2-bot commented Aug 8, 2019

amcgee commented Jun 11, 2019 •

edited

Loading

amcgee commented Jul 9, 2019 •

edited

Loading

varl commented Jul 10, 2019 •

edited

Loading

amcgee commented Jul 10, 2019 •

edited

Loading

amcgee commented Aug 8, 2019 •

edited

Loading