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

update to TypeScript v3.8-rc #57774

Closed
wants to merge 21 commits into from
Closed

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Feb 17, 2020

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:

For maintainers

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 17, 2020
@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/apm-ui I need your help here to fix APMRequestHandlerContext which type is inferred as any
2020-02-17_10-40-24

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looking into it!

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. It seems like it's not. On master I get this:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be related to the fact that Params interface contains optional props only.
When I remove an empty object default from

TParams extends Params = {}

TS starts inferring context type, but not params
2020-02-20_11-38-44

Copy link
Member

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.

Copy link
Contributor Author

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>();
Copy link
Contributor Author

@mshustov mshustov Feb 20, 2020

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',
Copy link
Contributor Author

@mshustov mshustov Feb 24, 2020

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.

"typescript": "3.7.2",
"typescript": "3.8.2",
Copy link
Contributor

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"]');
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

const [isOpen, setIsOpen] = useState();
const [isOpen, setIsOpen] = useState<boolean>();
Copy link
Contributor

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?

Copy link
Contributor Author

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

@mshustov mshustov added this to Code review in kibana-core [DEPRECATED] via automation Feb 25, 2020
@joshdover joshdover moved this from Code review to In progress in kibana-core [DEPRECATED] Feb 27, 2020
@kibanamachine
Copy link
Contributor

💔 Build Failed

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jfsiii
Copy link
Contributor

jfsiii commented May 5, 2020

Is this still in progress?

Worth noting 3.9 is an RC now and should be final in the next week or so.

@mshustov
Copy link
Contributor Author

mshustov commented May 5, 2020

Is this still in progress?

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.
prettier update PR #63735

Worth noting 3.9 is an RC now and should be final in the next week or so.

Yeah, but I'd prefer a gradual migration --> 3.8 --> 3.9 to minimize the amount of work.

@mshustov mshustov added v7.9.0 and removed v7.8.0 labels May 15, 2020
@mshustov
Copy link
Contributor Author

closed in favor of #67666

@mshustov mshustov closed this May 28, 2020
@mshustov mshustov deleted the ts-3.8-rc branch May 28, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants