Typescript strict #6566
Draft
Typescript strict #6566
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in order to typecheck correctly. this seems straightforward, and should actually be more performant since it doesn't create new closures, but needs extra attention in review.
…ual testing. update to latest typescript. gently bump troika to get a better type shape. de-fork three-to-ammo, the commits were merged and published upstream already. make a types-project that exports the strong types from the root dir project to the admin project, so that > [email protected] check > tsc will cross-validate admin's usage of hubs exports. inline a few local types where external package versions we're limited to don't have them.
… and put a visible vertical scrollbar on the content panel
- Logo stays fixed at top (never scrolls away) - Menu items scroll in dedicated area below logo - Single scroll area - no more double scrolling - Scroll indicators appear/disappear properly when content overflows - Works with Material-UI - no fighting the framework
… broken. fix height calculation issue that clipped the logo image incorrectly. verified bottom arrow still rendering correctly.
What: 1. Removes much of the AI styling changes. 2. Reverts a21f8d4 (show production hubs logo in the storybook...). 3. Changes the custom admin theme to inherit properties from the default theme. 4. Removes the sidebar text color override for mobile. 5. Fixes the issue with the sliding section of the light theme color at the top of the sidebar and ensures the dark theme color extends to the bottom of the sidebar (regardless of browser zoom). 6. Restores the appBar hiding functionality for desktop/large screens. 7. Fixes the issues with the sidebar scrolling by removing the scrolling wrapper and instead overriding the default react-admin sidebar with a custom version that has scrolling indicators that display whenever there is hidden content (either above or below the shown content). Why: 1. The AI styling changes seem to be unnecessary and to have contributed to the visual inconsistencies/glitches. 2. The "show production hubs logo in the storybook..." commit broke the display of the image in deployed instances. 3. Custom themes inheriting properties from the default theme is recommended in the React Admin documentation[1]. 4. The sidebar text color override for mobile is no longer needed (and causes readability issues) now that the background color is the same for both mobile and desktop. 5. It looks better to have the sidebar be a single consistent color from top to bottom. 6. This keeps the behavior of the appBar the same as it was before the Update storybook PR (Hubs-Foundation#6563) 7. There were two scrolling areas, which caused the scrolling to appear erratic, and the scrolling indicators weren't updated live (live updates ensures that people are always aware when the content can be scrolled and in which direction). [1] https://marmelab.com/react-admin/doc/3.19/Theming.html#sidebar-customization
…rom production scenario so storybook is a closer match. Decompose chrome into a separate file.
…ual testing. update to latest typescript. gently bump troika to get a better type shape. de-fork three-to-ammo, the commits were merged and published upstream already. make a types-project that exports the strong types from the root dir project to the admin project, so that > [email protected] check > tsc will cross-validate admin's usage of hubs exports. inline a few local types where external package versions we're limited to don't have them.
…. respect the brwoserlist for transpilation, rather than duplicating it implicitly with manual transpile rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Updates typescript, puts it into strict mode, and fixes numerous type checking errors throughout the codebase. Additionally, migrate to latest supported babel plugins to fix several issues that came up when closely inspecting the app bundle while verifying the typescript upgrade.
Why?
This will make future modifications more mistake-proof by catching common coding errors even before running the improved storybook tests.
Examples
Several issues found via manual QA testing of PRs this year could have been found earlier and with less effort with strict checking and latest typescript.
How to test
npm run checkwill run the full typecheck, this will also run during the standard build command.test deploying the app and testing on the now-raised lowest browser/device combinations. do the same testing for the admin panel.
Documentation of functionality
As discussed in PR #6563 , where to get a working pdfjs security fix (amongst other dependencies) upgraded, the minimum browser/OS version floor has been raised. Beyond that, users should see no difference except perhaps for better usage of browser built-ins and fewer polyfills being used, which may slightly improve FPS and battery life. This change is purely to ease ongoing maintenance of Hubs.
Limitations
Right now, we transpile several files to be compatible with Internet Explorer 11 so we can dynamically render an "unsupported browser" page. That page should be statically rendered, and the transpile to IE11 should be eliminated. This PR is already expansive, so I left it out, but added a TODO comment for a future adventurous person to give it a shot.
I also did not attempt to fully align the repo on ESM as part of this PR, which will require another jump forward in upgrades on several packages. The good news is that, which TS strict type checking on, API misalignments that occur in those future package upgrades will be caught earlier and cheaper in the local developer loop before anyone spends time manually testing deployments.
I added types in several places to eliminate ambiguity that typescript couldn't navigate by itself, but someone with a stronger opinion and more experience in the codebase could probably make them water-tight. For instance, all prop-types could now be eliminated and replaced with compile-type type checking ( a good AI coding assistant task for a new contributor to take on after this is merged).
Alternatives considered
Not really, it was all pretty straightforward. the tsconfig could be tightened up further, but I leave that to a future contributor.
Open questions
Additional details or related context