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

fix(ToolTip/index.ts): Exports TooltipProps as a type to help babel. #4854

Merged
merged 7 commits into from
Oct 1, 2020

Conversation

gradybknight
Copy link
Contributor

TooltipProps is exported but contains only TS code. Babel removes this at compile time throwing an
error with --isolatedModules flag.

4853

What: Closes #4853

Additional issues: n/a

TooltipProps is exported but contains only TS code. Babel removes this at compile time throwing an
error with --isolatedModules flag.

4853
@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 21, 2020

Copy link
Collaborator

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

It seems like 'TooltipProps' was incorrectly exported in the first place.
Seems that there are more cases in the codebase the Properties types are exported - but honestly I don't see the usecase for it.
Unless you have a usecase to use it, I would rather not export it.

…ort of TooltipProps type

Export of TooltipProps type causes issues with babel's --isolatedModules flag

4583
Copy link
Collaborator

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

This won't work either - CI fails with:

packages/react-table/src/components/Table/base/types.tsx:9:10 - error TS2305: Module '"../../../../../react-core/dist/js"' has no exported member 'TooltipProps'.

9 import { TooltipProps, PopoverProps } from '@patternfly/react-core';
           ~~~~~~~~~~~~

packages/react-table/src/components/Table/HeaderCellInfoWrapper.tsx:5:36 - error TS2305: Module '"../../../../react-core/dist/js"' has no exported member 'TooltipProps'.

5 import { Button, Tooltip, Popover, TooltipProps, PopoverProps } from '@patternfly/react-core';
                                    ~~~~~~~~~~~~

and much more errors since they expect in many places that that property is globally exported.

I am wondering if the correct approach is to remove it as you did and then import from direct file:

import { TooltipProps } from '@patternfly/react-core/src/components/Tooltip/Tooltip

in all internal usages of it.

Maybe we should just not export it as a type and put it bellow with the other exports. Maybe @redallen could give a better solution.

@gradybknight
Copy link
Contributor Author

Sorry about the multiple failures. I think the first solution was probably the most benign one: my understanding is that exporting as type (e.g. export type {TooltipProps} from './ToolTip'). This way all the files which point to index still have access to the type but Babel can safely compile it away as needed.

@KKoukiou
Copy link
Collaborator

Sorry about the multiple failures. I think the first solution was probably the most benign one: my understanding is that exporting as type (e.g. export type {TooltipProps} from './ToolTip'). This way all the files which point to index still have access to the type but Babel can safely compile it away as needed.

Sure - sorry for misleading. Would you like to also update packages/react-core/src/components/Popover/index.ts ?
It suffers from the same problem.

@gradybknight
Copy link
Contributor Author

Yep, will do. It's probably going to be at the end of the work day though (US time).

@redallen
Copy link
Contributor

redallen commented Sep 22, 2020

Hey @gradybknight,

I'm a little confused why you are using babel on @patternfly/react-core/src/components/Tooltip/index.ts. We provide transpiled JS in @patternfly/react-core/dist/esm/components/Tooltip/index.js or @patternfly/react-core/dist/js/components/Tooltip/index.js. This compiled JS does not include types as exports, but rather only export { Tooltip, TooltipPosition } from './Tooltip';.

You can easily support the "isolatedModules": true option if needbe in this PR by adding it to package/tsconfig.base.json and fixing the resulting errors from yarn build by separating type exports in index.ts files to export type { SomeType } from './Component';. There are only 3: Popover, Tooltip, and BaseLayout (in react-topology).

Thanks!

  • Zack

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020

Codecov Report

Merging #4854 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4854   +/-   ##
=======================================
  Coverage   52.73%   52.73%           
=======================================
  Files         525      525           
  Lines        9473     9473           
  Branches     3486     3486           
=======================================
  Hits         4996     4996           
  Misses       3848     3848           
  Partials      629      629           
Flag Coverage Δ
#patternfly4 52.73% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ages/react-core/src/components/Popover/Popover.tsx 42.47% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03d3fb9...0978be5. Read the comment docs.

jschuler
jschuler previously approved these changes Oct 1, 2020
@redallen redallen merged commit c7d4df2 into patternfly:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exported type in ToolTip
6 participants