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

Create TimeZone and Locale contexts #349

Closed
wants to merge 1 commit into from
Closed

Create TimeZone and Locale contexts #349

wants to merge 1 commit into from

Conversation

cherewaty
Copy link
Contributor

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 included ignoreLocalOffset: #329. ignoreLocalOffset could easily create a race condition with the timezone peeled off of stripes.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 and Timepicker were both tightly coupled to locale and timezone on the stripes context. By refactoring them to use the new Context API, we can make them both independent of stripes-core and significantly more testable.

Datepicker and Timepicker now simply have locale and timeZone props, that when inside of a LocaleContext.Provider/TimeZoneContext.Provider, get their value from context. The context can be overriden by the props.

I removed the ignoreLocalOffset prop. Instead, a Datepicker consumer can pass in the timeZone prop set to UTC.

Context providers

Newly introduced: Locale and TimeZone higher-order components, that include creation of the context provider and consumer for locale and timeZone.

Modules consuming stripes-components can also use the consumer HOCs to receive props from context:

import withTimeZone from '@folio/stripes-components/lib/TimeZone';

function ExampleComponent({ timeZone }) => (
  <div>{timeZone}</div>
);

export default withTimeZone(ExampleComponent)

Risk

Accessing this.context.stripes.locale and this.context.stripes.timezone should continue to work as previously, but we should find a way to deprecate those in favor of using withLocale and withTimeZone.

Questions for later

  • When in the context of a redux-form, Datepicker emits an ISO-860 string through onChange(), but when not in a redux-form, it fires an event whose target.value is the locale-formatted date in the input. Should those be more consistent? Should the Datepicker even try to reformat as ISO-860 by default?
  • Should setting moment.locale() happen in a more global place? A lot of the date utility functions defined in stripes-core can go away in that case, since they're usually just wrapping moment().

autoFocus: PropTypes.bool,
locale: PropTypes.string.isRequired,
timeZone: PropTypes.string.isRequired,
Copy link
Contributor

@JohnC-80 JohnC-80 Apr 27, 2018

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.

Copy link
Contributor Author

@cherewaty cherewaty Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use case for having an overridable timeZone prop for Datepicker: events. The user might have date and time pickers that they want to convert to another time zone just for the purposes of creating the record.

Example from Google Calendar:
2018-04-27 13 26 17

Copy link
Contributor

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.

@JohnC-80
Copy link
Contributor

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?

@cherewaty
Copy link
Contributor Author

@JohnC-80, if I understand your question correctly, the Datepicker still has PropTypes defined for locale and timeZone. If an HOC like withTimeZone tries to pass in a timeZone that's not a string, the Datepicker PropTypes validation will fail as usual.

@cherewaty
Copy link
Contributor Author

Last week @skomorokh mentioned a possibility to me: what if we created a CurrentUserContext that contained all the attributes currently supplied by context.stripes?

One of the advantages of the new context API is explicit subscriptions. Consumers only subscribe to the Providers they care about. Whenever the Provider value changes, the Consumer re-renders.

That makes it preferable to take the approach used here, where each attribute that used to live on context.stripes is now served by its own context Provider. Otherwise lots of components would go through unnecessary re-renders, since they'd be subscribing to an entire CurrentUserContext, not just the values they specifically need.

Having a Provider for every context attribute could make for a particularly verbose render() in stripes-core's Root.js, but should provide more clarity and better render performance everywhere else.

Copy link
Contributor

@skomorokh skomorokh left a 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.

@MikeTaylor
Copy link
Contributor

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.

Copy link
Contributor

@MikeTaylor MikeTaylor left a 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.)

@skomorokh
Copy link
Contributor

There is another aspect of this I want to touch on:

One of the benefits we get from having stripes-components as its own package is that apps can depend on different versions of it. While we ought rarely make breaking changes to component APIs, it is a nice quality that we can do so and, temporarily, let old modules bring in prior versions. This way we don't have to coordinate an update system-wide.

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 stripes-core or some new package that is consistently referred to as a peer dependency and not one where we want to maintain it as a regular dependency and tolerate multiple instances.

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?

@cowboyd
Copy link
Contributor

cowboyd commented May 1, 2018

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.

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:

There's nothing saying that a CurrentUserContext shouldn't exist, only that it exists as a platform concern, is defined by the platform, and it composes any number of subcontexts.

partly for conceptual reasons and partly for the pragmatic reason that I don't want to have to change a bunch of client code

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.

@MikeTaylor
Copy link
Contributor

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.

It's Stripes components. These components run in the context of Stripes. And Stripes gives you the Stripes object.

@cowboyd
Copy link
Contributor

cowboyd commented May 1, 2018

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.

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.

@cherewaty
Copy link
Contributor Author

So that we're all on the same page about what this would look like, here are some gists.

Scenario 1

https://gist.github.com/cherewaty/473f8e5e352b38afeea37e8086e780a7

  • CurrentUserContext
  • withCurrentUser() HOC that passes in whole currentUser object

Basically a direct translation of the current context.stripes strategy to the new Context API. End components have to know the structure of the currentUser object (it's stripesShape all over again). To test a component in isolation, it has to receive a prop whose currentUser is the right shape.

Scenario 2

https://gist.github.com/cherewaty/f60e7294639a2bf9f5139f090ea58c14

  • CurrentUserContext
  • withCurrentUser() HOC that spreads props

Slightly better than scenario 1, since end components don't have to know about a currentUser object, only now they're being sent props they didn't ask for. Could have strange side effects.

Scenario 3

https://gist.github.com/cherewaty/3426aac7fcb4286e637858fcc11697c7

  • CurrentUserContext
  • withTimeZone() HOC

Slightly better than scenarios 1 and 2, since the end component only receives the timeZone prop, but it's still subscribed to the entire currentUser context. That will cause a re-render when anything in currentUser changes, not just timeZone.

Scenario 4

https://gist.github.com/cherewaty/671afc60ebf250aff6cb18fcf8c2025d

  • TimeZoneContext
  • withTimeZone() HOC

What's proposed in this PR. Components subscribe only to the context they need, and can exist independently of any context. Root.js will end up with lots of providers, but that can be refactored to keep the list of providers isolated and readable.

@skomorokh
Copy link
Contributor

skomorokh commented May 1, 2018

As I brought up a time or two lately, I've come to understand that stripesShape is a misuse of proptypes. I think we'd derive more benefit from proptypes if they were used to delineate exactly the props that are actually consumed. If you only need timezone, specify CurrentUser as an object with only a timeZone property and your test is no harder to build.

And current user metadata is far from all that is in the stripes context or will eventually be, I was not proposing moving everything from stripes to it--I don't mind splitting it out into a few contexts (in addition to the user perhaps one for the current app, tenant, Okapi, etc.) But a context for every leaf property seems untenable. It's "lots of providers" to the extent that I anticipate dozens once our configuration is richer. To be fair to Mike, I struggle to justify splitting it at all but do see the benefits both in organisation and communicating something more specific than "stripes". Plus we can get it rolling faster so you can merge, trying to meet you part way.

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)

@cowboyd
Copy link
Contributor

cowboyd commented May 2, 2018

And current user metadata is far from all that is in the stripes context or will eventually be, I was not proposing moving everything from stripes to it--I don't mind splitting it out into a few contexts (in addition to the user perhaps one for the current app, tenant, Okapi, etc.) But a context for every leaf property seems untenable.

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.

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?

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.

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 2, 2018

@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.
Stripes-components only uses context.stripes in a handful of places...

  • IfPermission stripes.hasPerm
  • IfInterface stripes.hasInterface
  • Settings stripes.connect, stripes.hasPerm
  • Pluggable: stripes.plugins
  • MetaSection: stripes.formatDate, stripes.formatTime
  • AppIcon: stripes.metaData
  • Datepicker and
  • Timepicker as we know - stripes.locale and stripes.timezone

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.
Thinking out loud, these providers could be factored out to a single <StripesContextWrap> that would be instantiated in our Root and wrap its children in all of those Providers, so that stays tidy. The dev tools, as mentioned- already have unusable trees -but personally, I graduated to going straight for the search box a long time ago.
Settings looks like an interesting one, as it represents a point in the hierarchy where the stripes object comes out of context and becomes a prop so it warrants setting up a Provider for the entire stripes object because who knows how its routes are using it...

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.

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 2, 2018

I have not-so-warm feelings about exposing hasPerm and hasInterface as props... so that's 2 more we can scratch off - we'll have to figure out something else for those eventually.

@MikeTaylor
Copy link
Contributor

The React folks warned us for a while that the Context API was bound to change.

Act 1: 2016
Me: "I might stab you."
You: "Well, I'd rather you didn't."

Act 2: 2017
Me: (stabs you)
You: "Ouch!"

Act 3: later in 2017
Judge: You are accused of stabbing Mr. Coburn. How do you plead?
Me: guilty, but I did warn him that I might stab him.
Judge: Oh, OK then. Case dismissed.

@skomorokh
Copy link
Contributor

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 FooContext.js files each with their own withFoo export and their own invocation as a wrapper in Root.js.

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 withFoo().

Can't we get that in other ways?

import { fromUser } from 'stripes-core/contexts/user';

class Whatever extends component { ... }

export default fromUser(['locale', 'timezone'])(Whatever);

@MikeTaylor
Copy link
Contributor

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.

@skomorokh
Copy link
Contributor

@cowboyd what I mean by there can only be one is that there can only be one copy of FooContext.js. I like that we can currently use a module that depends on an old version of stripes-components and it just brings in a second copy at the old version and more or less works (though maybe the styles look dated or something).

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 stripes-components we lose this. Unless it's really just a static string like your CREATE_SOMETHING example and you can consume a context from a different file than the one which produced it, provided it's named the same?

Whereas, if it lives in stripes-core where it's primarily provided from, well, we already have to have only one copy of that. It does weird the dependency graph though and I've actively been trying to avoid having things import from core. However, I'm not super stoked about a new package for it either... I guess stripes-contexts wouldn't be the end of the world (but it might confuse people to think that it's not okay to have their own contexts within their app tree and otherwise make use of the mechanism when they certainly should be free to).

@skomorokh
Copy link
Contributor

@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.

@cherewaty
Copy link
Contributor Author

@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.

@MikeTaylor
Copy link
Contributor

@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.

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 3, 2018

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.

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 3, 2018

@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.

@cowboyd
Copy link
Contributor

cowboyd commented May 3, 2018

@skomorokh

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 stripes-components we lose this.

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 stripes-components library that consumes it. Note that contexts like user, institution, physical locations would not be part of stripes-components, because that's not its concern. For example, Let's say all the sudden a new version of stripes components comes out that moves the timezone context definition somewhere else. Well, since it both declares and consumes the timezone, if you try to upgrade a library that used the old constant, it will break. Precisely the behavior you want.

Whereas, if it lives in stripes-core where it's primarily provided from, well, we already have to have only one copy of that. It does weird the dependency graph though and I've actively been trying to avoid having things import from core. However, I'm not super stoked about a new package for it either... I guess stripes-contexts wouldn't be the end of the world (but it might confuse people to think that it's not okay to have their own contexts within their app tree and otherwise make use of the mechanism when they certainly should be free to).

I'm 100% behind the idea that the actual composition of the contexts lives in stripes-core, just as it does now. The only difference is that stripes-components defines only the primitives that it consumes. It says "Hey, if you want to pass me one of my props as a piece of context, just use this class. How (or even if) you choose to do that its totally your business".

So for example UserContext would be defined by stripes-core because it has the concept of a User (that would be a composition of locale, timezone, etc...). On the other hand, stripes-components which does not truck in users, would be remain in blissful ignorance.

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.

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 4, 2018

@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 React.createContext() generates. The behavior where the <Provider>-<Consumer> pair comes from each single call to React.createContext() creates concern if we have those pairs generated from multiple versions of stripes-components present in the system - stripes-core will only be serving its contextual value to a single instance of <Provider>.
If two modules exist in the system with differing versions of stripes-components (each setting up its own call to React.createContext(), I'm afraid it'll be a race as to which gets served a value first to feed its coordinating <Consumer>. The <Consumer> from the pair on the other version could get left in the dark.

@id-jenkins
Copy link
Contributor

@cowboyd
Copy link
Contributor

cowboyd commented May 7, 2018

@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.

@id-jenkins
Copy link
Contributor

yarn run v1.6.0
$ eslint lib && stylelint "lib/**/*.css"

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Datepicker/tests/Datepicker-test.js
13:7 error 'datepicker' is never reassigned. Use 'const' instead prefer-const

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Datepicker/tests/interactor.js
6:8 error Unable to resolve path to module '@bigtest/interaction' import/no-unresolved

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/EntrySelector/EntrySelector.js
76:92 error 'resources' is missing in props validation react/prop-types

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Locale/Locale.js
1:1 error Expected 1 empty line after import statement not followed by another import import/newline-after-import

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Locale/tests/Locale-test.js
8:8 error Unable to resolve path to module '@bigtest/interaction' import/no-unresolved
15:7 error 'LocaleComponent' is never reassigned. Use 'const' instead prefer-const
15:28 error 'locale' is missing in props validation react/prop-types
19:7 error 'LocaleComponentInteractor' is never reassigned. Use 'const' instead prefer-const
22:7 error 'testComponent' is never reassigned. Use 'const' instead prefer-const
24:7 error 'WrappedLocaleComponent' is never reassigned. Use 'const' instead prefer-const

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Timepicker/tests/interactor.js
6:8 error Unable to resolve path to module '@bigtest/interaction' import/no-unresolved

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Timepicker/tests/Timepicker-test.js
13:7 error 'timepicker' is never reassigned. Use 'const' instead prefer-const

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/Timepicker/Timepicker.js
161:31 error 'stripes' is missing in props validation react/prop-types
161:39 error 'stripes.locale' is missing in props validation react/prop-types

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/TimeZone/tests/TimeZone-test.js
8:8 error Unable to resolve path to module '@bigtest/interaction' import/no-unresolved
15:7 error 'TimeZoneComponent' is never reassigned. Use 'const' instead prefer-const
15:30 error 'timeZone' is missing in props validation react/prop-types
19:7 error 'TimeZoneComponentInteractor' is never reassigned. Use 'const' instead prefer-const
22:7 error 'testComponent' is never reassigned. Use 'const' instead prefer-const
24:7 error 'WrappedTimeZoneComponent' is never reassigned. Use 'const' instead prefer-const

/home/jenkins/workspace/folio-org/stripes-components/PR-349/project/lib/TimeZone/TimeZone.js
1:1 error Expected 1 empty line after import statement not followed by another import import/newline-after-import

✖ 21 problems (21 errors, 0 warnings)
12 errors, 0 warnings potentially fixable with the --fix option.

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@id-jenkins
Copy link
Contributor

@JohnC-80
Copy link
Contributor

JohnC-80 commented May 14, 2018

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.
For this, I'm definitely a fan of moving things towards prop-centric, dumber components with better isolation, but this will need to happen incrementally (as proposed in the JIRA). With points 1, 2 of the proposal in place, we can keep locale and timeZone props on Datepicker, with the addition of a stripes prop to catch context from a proposed <StripesContext.Provider>... with logic that would prefer either of those props (if provided) over the value from the consumed stripes object, so the timezone="utc" approach would work and could be passed either from a local state or from the stripes-providing ancestor. For testing purposes, the stripes object can just be set locally so that the override logic itself can be tested.

@cherewaty
Copy link
Contributor Author

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 stripes-components version. That's definitely the dealbreaker for this approach - stripes-core will need to continue being the one creating context.

@cherewaty cherewaty closed this May 14, 2018
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.

6 participants