Skip to content

Conversation

@priley86
Copy link
Member

@priley86 priley86 commented Mar 8, 2019

What:
This change disables allowSyntheticDefaultImports and esModuleInterop. From a library perspective, I believe we will need to ensure that we can support Typescript consumers who have this option disabled OR enabled. This was discovered today while testing OpenShift tests downstream.

It is generally considered good practice now to enable these flags (and from a Node JS/Common JS perspective) this is the typical path, however some consumers may not elect to enable them.

This should resolve the current issue and help us strategize for future TSX conversion.

cc: @dgutride @rhamilto @spadgett @tlabaj

For additional detail, with this flags disabled downstream, but enabled upstream, we'll run into these:

ERROR in /Users/priley/GitHub/web-ui/frontend/node_modules/@patternfly/react-core/dist/js/components/Avatar/Avatar.d.ts
(1,8): Module '"/Users/priley/GitHub/web-ui/frontend/node_modules/@types/react/index"' has no default export.

Additional issues:

@priley86 priley86 requested review from dgutride and tlabaj March 8, 2019 20:33
@rhamilto
Copy link
Member

rhamilto commented Mar 8, 2019

Tested and confirmed good. LGTM.

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://1535-pr-patternfly-react-patternfly.surge.sh

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #1535 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1535   +/-   ##
======================================
  Coverage    83.6%   83.6%           
======================================
  Files         551     551           
  Lines        5744    5744           
  Branches       12      12           
======================================
  Hits         4802    4802           
  Misses        940     940           
  Partials        2       2
Flag Coverage Δ
#patternfly3 84.89% <ø> (ø) ⬆️
#patternfly4 81.34% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...nfly-4/react-core/src/components/Avatar/Avatar.tsx 100% <100%> (ø) ⬆️

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 0b2905b...7bb74ce. Read the comment docs.

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Good for now

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

Should we remove the allowSyntheticDefaultImports?

dlabaj
dlabaj previously requested changes Mar 8, 2019
Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

Need to verify tree shaking and how this change effects other consumers prior to merging.

@dmiller9911
Copy link
Contributor

@dlabaj Note this will not have any effect on the treeshaking of React. React does not treeshake. All of their optimizations happens during their build process. They export a commonJS module (not ES) since they allow SSR in a node context. adding import * as React from 'react' is no different than import React from 'react'.

@priley86
Copy link
Member Author

priley86 commented Mar 8, 2019

For historical purpose with this PR, the proposed downstream change reverts the original proposed downstream change which would support the current configuration here today:
priley86/web-ui@971ecf9#diff-64282092d7b77f064e3f06a569429a99

@rhamilto
Copy link
Member

Happy Monday. I am waiting on a resolution to this PR in order to determine a course of action for OpenShift console. I want to make sure it doesn't get forgotten while @priley86 is out on PTO. Thank you.

@redallen redallen dismissed dlabaj’s stale review March 11, 2019 13:58

Tree shaking unchanged, default config line will be removed soon.

@redallen redallen merged commit 8107f5c into patternfly:master Mar 11, 2019
@redallen
Copy link
Contributor

@rhamilto Fixed in @patternfly/react-core 2.4.1

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.

8 participants