-
Notifications
You must be signed in to change notification settings - Fork 37
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
Create TimeZone and Locale contexts #349
Create TimeZone and Locale contexts #349
Conversation
autoFocus: PropTypes.bool, | ||
locale: PropTypes.string.isRequired, | ||
timeZone: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeZone
, while it works adequately with the React Context API, seems a little too open-to-suggestion for Datepicker. A developer will probably only ever have to pass 'UTC' to this prop, right? Any reason why we would want it to be open to any developer provided timezone?
Thank you for writing tests for this, and I definitely like having less dependency on stripes-core. just need this timezone aspect worked out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems viable to me.
For a hypothetical situation regarding the HOC, there wouldn't be any way for a wrapped component to apply its own component-level propType validation right? It has to use whatever the upper-level context creator uses - is that correct? It would have to be an independant wrapper that applied those validations to its developer-supplied props before overriding them with context- supplied values, right? |
@JohnC-80, if I understand your question correctly, the |
Last week @skomorokh mentioned a possibility to me: what if we created a One of the advantages of the new context API is explicit subscriptions. That makes it preferable to take the approach used here, where each attribute that used to live on Having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we gain sufficient utility from a separate context for each datum to warrant the overall fiddliness. If not just moving the Stripes
context into the new API, can we at least be a little less granular in how we break it down, perhaps just one for CurrentUser
?
The two main benefits to fragmenting things that I see are being clear about what consumes what and avoiding unnecessary re-renders when irrelevant information changes. Current user metadata is often used together (indeed, in some ways timezone is a subset of locale) and tends to update rarely, typically only at login or when user preferences are changed. I don't think these are frequent enough to provide a salient impact on the rendering of any particular view.
I don't think the verbosity will be restricted to Root.js
as the metadata about the current user expands as we add richness to user preferences:
- username
- timezone
- locale
- group membership
- permissions
- theme (and perhaps separately compact vs. standard view)
- hotkey metadata
- very many other things--we've really yet to scratch the surface of this since we currently don't persist preferences on a per-user basis
I'm up for splitting the current user metadata out from the stripes context (also lets us move to the new context API incrementally), but I really don't see what purpose is served in doing it atomically? Re-rendering is a moot point for such data as they do not change frequently nor in the midst of other interaction. Those few things that may eventually be so dynamic as to affect rendering should of course be in their own prop.
Folks, I don't want to see a proliferation of separate contexts, partly for conceptual reasons and partly for the pragmatic reason that I don't want to have to change a bunch of client code. I recognise that the new React context API(*) means we're going to need to stop using the old one when we reach React 17, but the simple upgrade path is just to provide a single context that furnishes the existing Stripes object, exactly as it presently is. Let's do that now; if we later decide we additionally want some more fine-grained contexts within that, we can fault them in as needed. (*) A pox on the React developers, and indeed all JS developers, for their idiot habit of constantly churning their APIs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(null comment, so I can register the fact that I am requesting changes. For some idiot reason, GitHub won't let me just say "Request changes" in light of my previous comment.)
There is another aspect of this I want to touch on: One of the benefits we get from having The part of the new context app I find most disturbing is the reliance on every subsequent import of the context getting a reference to the object created on the first import. Effectively a webpack-mediated global variable. Much like React and Highlander, this means we can only have one. So I think we're best off importing the context from Maybe now we can finally establish a coherent practice for such peer deps--does the CI install them? Or always use stripes-cli to test? Or we cross-list them as dev dependencies? |
As I understand it, the goal of stripes components is to be a component library and design system, which means that for components to be composable, they need to make as little assumption about their containing environment as possible. Higher coupling = Lower composability = less component, more code util. All things being equal, shouldn't we opt for higher composability in our component library? In this instance for example, the concept of a user feels like an application concern, not a component concern. The component, a datepicker, needs a timezone and locale to do its job. Why couple that to a user? It's not to say that the coupling won't happen somewhere. Clearly, it must, but it's a question of where. Isn't the most logical place to define the coupling between user, timezone and locale the very same place that defines the concept of user in the first place? Why duplicate this coupling across both stripes-core and stripes-components? Among other things, It seems to me a very plausible use case to want to render a date picker without a user.
There's nothing saying that a
If the concern here is backwards compatibility, then I think that's definitely doable that we can have a way forward without breaking existing code. |
It's Stripes components. These components run in the context of Stripes. And Stripes gives you the Stripes object. |
I don't think I understand. Doesn't the same apply to a function or any other constant defined within a module? // there can be only one (function named createSomething in this module)
export function createSomething() {
//....
}
// there can be only one (constant named CREATE_SOMETHING in this module)
export const CREATE_SOMETHING = "CREATE_SOMETHING";
// there can be only one (constant named SomethingContext in this module)
export const SomethingContext = React.createContext(); It's just a constant that identifies a context, so that it can be consumed and provided. |
So that we're all on the same page about what this would look like, here are some gists. Scenario 1https://gist.github.com/cherewaty/473f8e5e352b38afeea37e8086e780a7
Basically a direct translation of the current Scenario 2https://gist.github.com/cherewaty/f60e7294639a2bf9f5139f090ea58c14
Slightly better than scenario 1, since end components don't have to know about a Scenario 3https://gist.github.com/cherewaty/3426aac7fcb4286e637858fcc11697c7
Slightly better than scenarios 1 and 2, since the end component only receives the Scenario 4https://gist.github.com/cherewaty/671afc60ebf250aff6cb18fcf8c2025d
What's proposed in this PR. Components subscribe only to the context they need, and can exist independently of any context. |
As I brought up a time or two lately, I've come to understand that And current user metadata is far from all that is in the With all the HOCs the component tree is already extremely nested and thus less useful for debugging and understanding the app than it might otherwise be. Perhaps the devtools are being improved to better collapse HOCs together? (Sorry Charles, I have to run, I'll get to your comment on Thursday) |
Why is this untenable? It results components that are simpler, performant by default, and most important, more composable. Also as a clear benefit, there's a plain, one-way dependency from stripes-core to stripes-components instead of the circular one we have now. In the days when providing and consuming contextual data in React involved a ton of ceremony, rolling our own ad-hoc context layer made more sense. We weren't the only ones though, and the new context API was rolled out precisely as a countermeasure against the accretion of global contexts like the Stripes object that reduce performance, reactivity, and composability.
I was looking at our application yesterday in the react devtools, and our component tree is well-nigh unusable as it is, but it does look like this discussion has been going on for a very long time within the React community and that help is very soon on the way. The fact that there has been so much interest in this feature, and so much involvement from the core team is a strong indicator that this is considered a good practice. Given that it appears that the devtools are moving in this direction and so it shouldn't be a factor in our decision. |
@MikeTaylor The React folks warned us for a while that the Context API was bound to change. Sure, there's pain up front of converting our injected context to use prop-based context... but I feel like this was the inevitable path for React since the component-props relationship is at the core of how it's used to build UI's. Best for them to draw closer to that model than persisting the outlier.
10 distinct keys we could start with... 2 are done for us. I'd like for these components to only consume the pieces they care about and not be fed unsolicited props. The old way will be around until v17, so not an intense, forced rush and not like they couldn't exist in both places - but you never know when we might start seeing those glorious deprecation console warnings. |
I have not-so-warm feelings about exposing |
Act 1: 2016 Act 2: 2017 Act 3: later in 2017 |
I do find the idea of a more honest API where consuming components aren't coupled to unnecessary application detail compelling. The part where I find this untenable is that, by the time we have fleshed out the metadata for the tenant, the application, the user, the app, okapi, etc. we will have several dozen properties in play. Let's say a nice round 100. While it feels a bit dangerous to assume, I imagine there isn't really a huge performance impact to 100 wrappers as React is heavily optimized. And if that PR Charles linked to is likely to be merged, we can tidy up the dev tools. Ifs, but not such big ifs as the zeitgeist is blowing this way. But I'd like to hear more about how we plan to manage providing them. Because at the moment I envision 100 near-identical The maintainability issue there feels like it would take some heroics to tame. Flattening the application detail out of the context props seems comparatively trivial. It seems like the main benefit isn't from each of these being separate contexts (as the rendering issue is moot for the vast majority) but rather from well-specified component APIs enabled by the granularity of having Can't we get that in other ways?
|
All of this feels like a ton of work to yield an effect whose value is very close to zero -- whether slightly positive or slightly negative is not completely clear. Don't we have a million other, more important and urgent, things to be doing? I'm reminded of the zinger at the end of #8 in Henry Spencer's Ten Commandments for C Programmers: "thy creativity is better used in solving problems than in creating beautiful new impediments to understanding". That observation must be 30 years old at the very outside, but it's as true now as it ever was. |
@cowboyd what I mean by there can only be one is that there can only be one copy of That makes upgrades a whole lot less painful because we don't have to coordinate new versions of everything at once. And the extra copies are tolerable with async loading. If we store the context in Whereas, if it lives in |
@MikeTaylor this is important in the short term for better testability. Tests are important. They really are. In the long term, we need to revisit this anyway because our tools are improving underfoot. Context forms one of the main APIs consumed by Stripes developers and so it's well worth us putting some thought into how we structure it. It also improves the API for the myriad components which consume it by making it clear what they need and thus facilitating reuse. |
@MikeTaylor "This isn't my priority, why is it yours?" is an unacceptable response to contributions in an open-source community. The project is strengthened by contributors whose diverse priorities converge on a solution that benefits all. Snarky attempts to shut down constructive collaboration should not, and will not, be tolerated in this community. |
@cherewaty It's very considerate of you to tell us what will and will not be tolerated in our own community. The "solution" that you propose is one that will have implications for all of us, changing code that presently works. I reject your claim that I'm not allowed to object to that. And with that, I am out of this conversation. You guys can sort it out between you, and do whatever rewriting of the existing code may be necessary as a result. |
Guys, let's stay on some task here. @MikeTaylor I think there's awareness of the implications there - @cherewaty did note the risk in his description. That's why these PR's exist so that we can talk through these things and find solutions to whichever concerns there are. While it's true that we've got a scope that can drown things in priority, this pattern of context affects how we'll do things going forward. We can't keep adding keys to the old context and behave like the new API isn't a thing we should be concerned about. Ideally, we find positives in producing code with less dependencies on things like the shape of an object that's produced higher in the component tree as well as the niceness of testing components as it's all props now, rather than injected outsider values that have to be shimmed together in some sort of harness in order for testing to even happen properly. Yes, it's a good deal of work to turn the battleship, and it would be nice to find ways to mitigate the workload to hopefully set us up for fewer occurrences of it in the future. |
@skomorokh Yeah, there's gotta be a better way to do it than having custom HOC's for every key on the contextualized object. It would be nice to have a way for the component to pass through the particular pieces of context it needs in order to receive those as its props... hmmm. |
Wouldn't it actually do a better job at achieveing api compatibility? The reason is because the version of the context you need is bundled with the same version of the
I'm 100% behind the idea that the actual composition of the contexts lives in So for example It's really the same idea as composition of simple objects. JavaScript gives us Strings, Numbers, Dates and things which we can use to build Users, Addresses, Cars, what have you. The primitives live in one place, and then the app uses them to express the higher level concepts that it works with. The guiding rule I'd see of where these things would go would be, if you define the concept, supply the context. |
@cowboyd I think we would want to define these global contexts and their components in core or as high as we can to get a single source of the component pairs that a single call to |
Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/stripes-components/job/PR-349/5/ |
@skomorokh @JohnC-80 I see what you're saying now. I had not accounted for having multiple versions of stripes-components being used at the same time. In light of this, I think we should back away from the context portion for the moment, and just focus on the props. |
yarn run v1.6.0 $ eslint lib && stylelint "lib/**/*.css" |
Test errors found. See https://jenkins-aws.indexdata.com/job/folio-org/job/stripes-components/job/PR-349/6/ |
I've created a JIRA for conversation/proposal of new context approach Ideally this brings us to an iterative way to embrace the new context API. |
Closing, since large changes will need to be made based on the conversation moved to https://issues.folio.org/browse/STCOR-208. Thanks @JohnC-80 for moving that over and pushing this forward. Thanks @skomorokh for pointing out that every module can have its own |
Purpose
Work for https://issues.folio.org/browse/UIORG-55 changed the behavior of
Datepicker
. Instead of emitting an ISO-8601 datetime string in the user's local time zone, it now emits the datetime at UTC.Resulting bug in eHoldings: https://issues.folio.org/browse/STCOM-259
One of the APIs added to the
Datepicker
includedignoreLocalOffset
: #329.ignoreLocalOffset
could easily create a race condition with thetimezone
peeled off ofstripes.context
.All of these need tests. We should be able to more easily identify breaking changes.
Approach
Testing context with the new React Context API is muuuuuch easier than before.
Datepicker
andTimepicker
were both tightly coupled tolocale
andtimezone
on thestripes
context. By refactoring them to use the new Context API, we can make them both independent ofstripes-core
and significantly more testable.Datepicker
andTimepicker
now simply havelocale
andtimeZone
props, that when inside of aLocaleContext.Provider
/TimeZoneContext.Provider
, get their value from context. The context can be overriden by the props.I removed the
ignoreLocalOffset
prop. Instead, aDatepicker
consumer can pass in thetimeZone
prop set toUTC
.Context providers
Newly introduced:
Locale
andTimeZone
higher-order components, that include creation of the context provider and consumer forlocale
andtimeZone
.Modules consuming
stripes-components
can also use the consumer HOCs to receive props from context:Risk
Accessing
this.context.stripes.locale
andthis.context.stripes.timezone
should continue to work as previously, but we should find a way to deprecate those in favor of usingwithLocale
andwithTimeZone
.Questions for later
redux-form
,Datepicker
emits an ISO-860 string throughonChange()
, but when not in aredux-form
, it fires an event whosetarget.value
is the locale-formatted date in the input. Should those be more consistent? Should theDatepicker
even try to reformat as ISO-860 by default?moment.locale()
happen in a more global place? A lot of the date utility functions defined instripes-core
can go away in that case, since they're usually just wrappingmoment()
.