Issue 661 Tech Radar Typescript refactor #1080
Merged
Conversation
This file contains 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
Seeing some errors that I don't fully understand around the proptypes assertions seemingly being treated as null | undefined values. It looks like I'll need to type Segment, which is not explicitly in-scope for this issue.
I incorrectly added type safety to the original implementation, causing the segments to no longer be populated here. This change fixes that regression while retaining the type safety. This is all necessary because qidx and ridx are apparently both nullable.
A global search looks like no other plugins use PropTypes. I'm about to remove them in the next commit because shapes don't seem to cooperate with TypeScript, making the PropTypes either so vague that they're mostly useless or specific enough to fail the build. With that said, I wanted to get this version committed in case I need to roll back to here. I wouldn't want to redo this work.
The build now passes.
I could see these being split into separate files, but I think this is enough for now.
I'm not confident in this change. I'm very unclear about the reasoning behind the type definitions in api.ts as they don't reflect the shape of the values that I see when I print `data.entries` in the console. The value of data.entries reflects a fully-populated Entry[], not a RadarEntry[]. As such, I'm also not sure about the sample data. I would be more hesitant to updated it, but the fact that the rest of the components seem to be expecting that data makes it seem fairly clear that it must be required to be in the updated shape.
Rugvip
approved these changes
Jun 1, 2020
benjdlambert
added a commit
that referenced
this pull request
Jun 8, 2020
…entities * 'master' of github.com:spotify/backstage: (87 commits) Minor tweaks to catalog UI (#1176) plugins/tech-radar: fix lint issue in RadarBubble Fix backend startup instructions (#1188) Changed order of backend install instructions Issue 661 Tech Radar Typescript refactor (#1080) build(deps-dev): bump eslint-plugin-cypress from 2.10.3 to 2.11.1 (#1184) build(deps): bump eslint-plugin-import from 2.20.2 to 2.21.1 (#1183) Fix backend not starting (#1185) fix page reload when clicking on logo (#1178) Add background color for tabs (#1170) chore(navigation/tabs): fixing prop type validation (#1173) Change example component names (#1165) chore(Catalog/Tabs): remove extraneous file from otber branch packages/cli: avoid duplicate dependency warning in E2E test build(deps): [security] bump websocket-extensions from 0.1.3 to 0.1.4 feat(Catalog/tabs): added some sample tabs for now Added HeaderTabs to ComponentPage fix: import from package fix: type checking fix: tests ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hey, I just made a Pull Request!
If successful, this PR has no effect on the end-user.
Change Summary
Notes on the Approach
I have taken a very naive approach to this refactor. In an effort to limit the scope of the change and potential regressions, I have opted to:
null|undefineddefinition even when the component itself seemed to assume this was not the case).I removed the PropTypes from Tech Radar as it appeared to be the only plugin still using PropType validation, which did not cooperate very well with TypeScript. If this is not a desirable change, please advise.
Future Work
Segmentclass to use TypeScript✔️ Checklist
yarn test