Update dependencies #575
Update dependencies #575
Conversation
| props.children | ||
| ); | ||
| } | ||
|
|
||
| LocalizationProvider.propTypes = { |
There was a problem hiding this comment.
Question: Is this a behavior change for our users? Not everyone uses TypeScript, and this could provide valuable runtime checks if users are relying on the added safety here.
There was a problem hiding this comment.
The prop-types methods are effectively no-ops in production, so I don't think that this should be considered a change in behaviour. It's removed here because issues like facebook/prop-types#354 make it difficult to support react@17 while keeping it as a dependency.
There was a problem hiding this comment.
I would somewhat disagree with the change in behavior, as it's still used in development mode. However, I won't block on this change if you feel the dependency management is too complex.
| @@ -25,18 +25,4 @@ describe("LocalizationProvider - validation", () => { | |||
| ); | |||
| expect(renderer.toJSON()).toMatchInlineSnapshot(`<div />`); | |||
| }); | |||
|
|
|||
| test("without a child", () => { | |||
There was a problem hiding this comment.
Suggestion (blocking): I'm nervous about this change, since we can't do this with TypeScript type checks, but our users might. For instance in Firefox DevTools this is used without type checks.
There was a problem hiding this comment.
This PR drops two tests. They expected these to throw:
<LocalizationProvider l10n={new ReactLocalization([])} /><LocalizationProvider />I don't think there's any actual benefit from throwing for these, and doing so is in fact surprising. A LocalizationProvider is a Context.Provider, which normally does not require its children to be non-empty. It just does nothing in that case.
The second checks that the l10n prop is defined. This will continue to throw in actual use, as the users of the context get its value with the TS type ReactLocalization | null, and handle the latter by throwing. This is already effectively checked by tests in fluent-react/test/localized_valid.test.js.
@gregtatum Would you like me to add a more specific test to the Localized test suite covering the missing-prop case?
There was a problem hiding this comment.
I think there is some value in exercising these code paths as users of this package could do surprising things with it. Without the throws, I'm less opinionated if the tests stay or if they go.
fluent-dom/test/index.js
Outdated
| @@ -13,3 +13,6 @@ Object.keys(document.defaultView).forEach(property => { | |||
| global.navigator = { | |||
| userAgent: 'node.js' | |||
| }; | |||
|
|
|||
| // silence console logging during tests | |||
| console.warn = () => {} | |||
There was a problem hiding this comment.
Suggestion: I don't think this is a good change. It would be better to add suppressions per-test. This is a global change for every test, and it could be very surprising as to why this function doesn't work. I generally treat console as a public API, and one that should be handled explicitly, and asserted in tests.
There was a problem hiding this comment.
Ok, will change. I'll bump up sinon to the root package.json and use that in these tests as well.
This updates almost all of the project's dependencies, including the following major dev dependency updates:
rollupv1 → v2typedocv0.20 → v0.22typescriptv4.2 → v4.4sinonv4 → v11 (fluent-bundle)jsdomv15 → v17 (fluent-dom)jestv24 → v27 (fluent-react)reactv16 → v17 (fluent-react)Some of those require a few changes in code; these are also applied. The following externally visible changes are made:
prop-typesis dropped as a dependencyThe version of
commanderthat's used intools/is not updated here, as the distance in its updates (v2 → v8) is so great, and the tools aren't covered by any tests.