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
update to TypeScript v3.8-rc #57774
update to TypeScript v3.8-rc #57774
Conversation
@@ -15,7 +15,7 @@ | |||
"js-yaml": "^3.13.1", | |||
"p-limit": "^2.2.1", | |||
"ts-loader": "^6.1.0", | |||
"typescript": "3.7.2", | |||
"typescript": "3.8.1-rc", |
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.
@dgieselaar fyi
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.
Looking into it!
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.
@restrry I can't explain this - there's no reason I can see why this fails, and others don't. It feels like a TS bug, but I cannot pinpoint it.
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.
btw - in my case, I don't see context
as any, but the type for context.params
is incorrect.
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.
@sqren is context.params
type correct in your case?
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.
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'm not here until Monday, and my only guess right now is that this is a TS bug (either in resolving or in showing an appropriate error). Can someone flag this with the TS team? If there's not much of a rush I can look into it Monday.
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.
@dgieselaar take your time, we are blocked by external dependencies as well
@@ -70,18 +70,18 @@ const emptyCaption = '<br>'; | |||
function Panel({ interval, seriesList, renderComplete }: PanelProps) { | |||
const kibana = useKibana<TimelionVisDependencies>(); | |||
const [chart, setChart] = useState(() => cloneDeep(seriesList.list)); | |||
const [canvasElem, setCanvasElem] = useState(); | |||
const [chartElem, setChartElem] = useState(); | |||
const [canvasElem, setCanvasElem] = useState<HTMLElement>(); |
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.
the file still contains some type errors. @timroes @sulemanof could someone take a look and fix the errors, please?
import * as testingLibraryDom from '@testing-library/dom'; | ||
|
||
testingLibraryDom.configure({ | ||
testIdAttribute: 'data-test-subj', |
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 got a bunch of type errors due to data-testid
not being declared on EUI elements. Our component library shouldn't depend on 3rd party libraries naming conventions, so I reconfigured @testing-library
to use Kibana specific data-test-subj
attribute instead.
packages/kbn-pm/package.json
Outdated
"typescript": "3.7.2", | ||
"typescript": "3.8.2", |
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.
(Thinking) npm really need a maven-like pinable version management for multi-projects...
const importButton = wrapper.find('[data-testid="ml_new_event"]'); | ||
const importButton = wrapper.find('[data-test-subj="ml_new_event"]'); |
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.
Just wondering, as these are JS files: Did you manually grep for data-testid
to 'fix' that ?
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.
yes
x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.test.tsx
Outdated
Show resolved
Hide resolved
const [isOpen, setIsOpen] = useState(); | ||
const [isOpen, setIsOpen] = useState<boolean>(); |
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.
To understand, are these added explicits types are a requirement for the bump or only cleanup?
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.
To understand, are these added explicits types are a requirement for the bump
yeah, it started showing error after TS version upgrade
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
Is this still in progress? Worth noting 3.9 is an RC now and should be final in the next week or so. |
Yes. We have to update prettier to v2 to support new syntax, which requires code style changes on the whole codebase. I will get back to work after FF.
Yeah, but I'd prefer a gradual migration --> 3.8 --> 3.9 to minimize the amount of work. |
closed in favor of #67666 |
Summary
This PR starts preparatory work to upgrade to TS
v3.8
. Changes: https://devblogs.microsoft.com/typescript/announcing-typescript-3-8-beta/Blockers
External:
Internal:
apm
plugin @elastic/apm-uicytoscape
&@types/cytoscape
@elastic/apm-uitimelion
plugin @elastic/kibana-appFor maintainers