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(docs): documented CJS tree shaking solution for icons and react-core #3410

Merged
merged 2 commits into from Dec 16, 2019

Conversation

@evwilkin
Copy link
Contributor

evwilkin commented Dec 16, 2019

What: Addresses #3268

Additional issues:

@evwilkin evwilkin requested a review from redallen Dec 16, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 16, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 16, 2019

Codecov Report

Merging #3410 into master will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3410      +/-   ##
==========================================
+ Coverage   67.12%   67.44%   +0.32%     
==========================================
  Files         903      897       -6     
  Lines       25442    25173     -269     
  Branches     2243     2190      -53     
==========================================
- Hits        17079    16979     -100     
+ Misses       7332     7170     -162     
+ Partials     1031     1024       -7
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (-0.01%) ⬇️
#patternfly4 64.86% <ø> (+0.6%) ⬆️
Impacted Files Coverage Δ
...tternfly-4/react-core/src/components/List/List.tsx 83.33% <0%> (-4.67%) ⬇️
...eact-core/src/components/Dropdown/DropdownItem.tsx 68.42% <0%> (-3.01%) ⬇️
...eact-core/src/components/Dropdown/DropdownMenu.tsx 51.85% <0%> (-2.39%) ⬇️
...ponents/Modal/InnerComponents/CustomModalDialog.js 64.4% <0%> (-1.67%) ⬇️
...ApplicationLauncher/ApplicationLauncherContent.tsx
...ts/ApplicationLauncher/ApplicationLauncherText.tsx
...ts/ApplicationLauncher/ApplicationLauncherItem.tsx
...s/ApplicationLauncher/ApplicationLauncherGroup.tsx
...ts/ApplicationLauncher/ApplicationLauncherIcon.tsx
...plicationLauncher/ApplicationLauncherSeparator.tsx
... and 5 more

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 2b96cde...91096c8. Read the comment docs.

@dlabaj
dlabaj approved these changes Dec 16, 2019
@dlabaj dlabaj merged commit 7e7eb01 into patternfly:master Dec 16, 2019
9 checks passed
9 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_a11y_pf4 Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
To enable tree shaking with named imports for CJS modules, utilize [babel-plugin-transform-imports](https://www.npmjs.com/package/babel-plugin-transform-imports) and update a babel.config.js file to utilize the plugin:
```JS
require.extensions['.css'] = () => undefined;
const components = require('@patternfly/react-core/dist/js/components');

This comment has been minimized.

Copy link
@jschuler

jschuler Dec 16, 2019

Collaborator

our package.json files have main and module entries, when doing require it should find the cjs entry point as specified in main and when doing imports it should find the esm entry point in module. So I don't think you need the whole path?

This comment has been minimized.

Copy link
@jschuler

jschuler Dec 16, 2019

Collaborator

anyways, might be different for babel js resolution. looks good :)

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 16, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.56
  • @patternfly/react-core@3.128.2
  • @patternfly/react-docs@4.16.67
  • @patternfly/react-inline-edit-extension@2.14.16
  • demo-app-ts@3.15.2
  • @patternfly/react-table@2.24.60
  • @patternfly/react-topology@2.11.44
  • @patternfly/react-virtualized-extension@1.3.57
  • @patternfly/react-icons@3.14.27

Thanks for your contribution! 🎉

@Hyperkid123

This comment has been minimized.

Copy link
Contributor

Hyperkid123 commented Dec 17, 2019

@evwilkin @dlabaj @redallen Hello, I did some testing and there are some issues with this particular config. Adding link to a issue with more detail: #3268 (comment)

@martinpovolny

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Dec 17, 2019

@evwilkin : First on all I'd like to thank you for keeping working on this. I really appreciate that despite what I am going to write next.

So I have to say that this issue makes me wonder about Patternfly development processes.

You have an issue that is complex, hard to fix, hard to understand, hard to discuss.

You discuss it for weeks. Have ideas, some wrong, some right, obviously it's not a simple thing.

The issue was already once prematurely closed and reopened.

Then you make a PR and merge it within 1 hour. Merge it within one hour since it was opened w/o giving the people who where previously discussing the issue a chance to check it/test it/review it.

Well and no. It did not resolve the issue.

Wow. Just wow.

@evwilkin

This comment has been minimized.

Copy link
Contributor Author

evwilkin commented Dec 17, 2019

@martinpovolny please note that we've kept the original issue open (#3268 ) until we're able to validate together that we have a successful fix, and are happy to continue working together to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.