Skip to content

update to TypeScript v3.8-rc#57774

Closed
mshustov wants to merge 21 commits intoelastic:masterfrom
mshustov:ts-3.8-rc
Closed

update to TypeScript v3.8-rc#57774
mshustov wants to merge 21 commits intoelastic:masterfrom
mshustov:ts-3.8-rc

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
"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
Contributor

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
Contributor

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
Contributor

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
Contributor

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

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.

"strong-log-transformer": "^2.1.0",
"tempy": "^0.3.0",
"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 wrapper = mountWithIntl(<NewCalendar {...props} />);

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

export const WaffleAccountsControls = (props: Props) => {
const { accountId, options } = props;
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

@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

None yet

Development

Successfully merging this pull request may close these issues.

8 participants