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

Default pragma #229

Merged
merged 3 commits into from
Sep 26, 2018
Merged

Default pragma #229

merged 3 commits into from
Sep 26, 2018

Conversation

pcvonz
Copy link
Contributor

@pcvonz pcvonz commented Aug 13, 2018

This PR is a:

[ ] New feature
[ ] Enhancement/Optimization
[x] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Switch back to the default pragma in our Babel configuration. More information on why this switch was made originally and why we're moving back are documented here

To sum it up though: We need to add something to our compiler to let it know how to compile JSX. A decision was made to stray away from the standard way. This pull request undoes that and implements the standard way. (Hope that sums it up well)

NOTE: This pull request is tied to this another pull request in magento-eslint:
magento/magento-eslint#1

Additional information

Fixes #21

@coveralls
Copy link

coveralls commented Aug 13, 2018

Pull Request Test Coverage Report for Build 910

  • 0 of 59 (0.0%) changed or added relevant lines in 59 files are covered.
  • 34 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.8%) to 30.824%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/peregrine/src/ContainerChild/ContainerChild.js 0 1 0.0%
packages/peregrine/src/List/item.js 0 1 0.0%
packages/peregrine/src/List/items.js 0 1 0.0%
packages/peregrine/src/List/list.js 0 1 0.0%
packages/peregrine/src/Peregrine/Peregrine.js 0 1 0.0%
packages/peregrine/src/Price/Price.js 0 1 0.0%
packages/peregrine/src/Router/MagentoRouteHandler.js 0 1 0.0%
packages/peregrine/src/Router/Router.js 0 1 0.0%
packages/peregrine/src/util/fromRenderProp.js 0 1 0.0%
packages/venia-concept/src/RootComponents/CMS/CMS.js 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
packages/venia-concept/src/components/Header/header.js 3 0.0%
packages/venia-concept/src/RootComponents/Category/category.js 4 0.0%
packages/venia-concept/src/components/CategoryList/categoryList.js 9 0.0%
packages/venia-concept/src/components/Gallery/item.js 18 0.0%
Totals Coverage Status
Change from base Build 901: -0.8%
Covered Lines: 519
Relevant Lines: 1691

💛 - Coveralls

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Aug 16, 2018

Fails
🚫

The following unit tests did not pass 😔. All tests must pass before this PR can be merged

packages/venia-concept/src/components/Gallery/__tests__/item.spec.js
�[1m�[31m  �[1m● �[1mrenders Anchor elements for navigating to a PDP�[39m�[22m
ReferenceError: React is not defined

�[2m�[22m
�[2m �[0m �[90m 6 | �[39m�[90m * currently by rendered in the test environment�[39m�[0m�[22m
�[2m �[0m �[90m 7 | �[39m�[90m */�[39m�[0m�[22m
�[2m �[0m�[31m�[1m>�[2m�[39m�[90m 8 | �[39m�[36mexport�[39m �[36mconst�[39m �[33mPrice�[39m �[33m=�[39m () �[33m=>�[39m �[33m<�[39m�[33mdiv�[39m �[33m/�[39m�[33m>�[39m�[33m;�[39m�[0m�[22m
�[2m �[0m �[90m | �[39m �[31m�[1m^�[2m�[39m�[0m�[22m
�[2m �[0m �[90m 9 | �[39m�[0m�[22m
�[2m�[22m
�[2m �[2mat Object..exports.Price (�[2m�[0m�[36msrc/mocks/@magento/peregrine.js�[39m�[0m�[2m:8:28)�[2m�[22m
�[2m �[2mat mountIndeterminateComponent (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:13744:13)�[2m�[22m
�[2m �[2mat beginWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:14068:16)�[2m�[22m
�[2m �[2mat performUnitOfWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:16415:12)�[2m�[22m
�[2m �[2mat workLoop (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:16453:24)�[2m�[22m
�[2m �[2mat renderRoot (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:16532:7)�[2m�[22m
�[2m �[2mat performWorkOnRoot (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17386:7)�[2m�[22m
�[2m �[2mat performWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17294:7)�[2m�[22m
�[2m �[2mat performSyncWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17266:3)�[2m�[22m
�[2m �[2mat requestWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17154:5)�[2m�[22m
�[2m �[2mat scheduleWork (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:16948:5)�[2m�[22m
�[2m �[2mat scheduleRootUpdate (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17636:3)�[2m�[22m
�[2m �[2mat updateContainerAtExpirationTime (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17663:10)�[2m�[22m
�[2m �[2mat updateContainer (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17690:10)�[2m�[22m
�[2m �[2mat ReactRoot.Object..ReactRoot.render (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17956:3)�[2m�[22m
�[2m �[2mat �[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:18096:14�[2m�[22m
�[2m �[2mat unbatchedUpdates (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:17517:10)�[2m�[22m
�[2m �[2mat legacyRenderSubtreeIntoContainer (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:18092:5)�[2m�[22m
�[2m �[2mat Object.render (�[2m�[0m�[36m../../node_modules/react-dom/cjs/react-dom.development.js�[39m�[0m�[2m:18151:12)�[2m�[22m
�[2m �[2mat Object.render (�[2m�[0m�[36m../../node_modules/enzyme-adapter-react-16/build/ReactSixteenAdapter.js�[39m�[0m�[2m:314:114)�[2m�[22m
�[2m �[2mat new ReactWrapper (�[2m�[0m�[36m../../node_modules/enzyme/build/ReactWrapper.js�[39m�[0m�[2m:119:16)�[2m�[22m
�[2m �[2mat mount (�[2m�[0m�[36m../../node_modules/enzyme/build/mount.js�[39m�[0m�[2m:21:10)�[2m�[22m
�[2m �[2mat Object..test (�[2m�[0m�[36msrc/components/Gallery/tests/item.spec.js�[39m�[0m�[2m:63:21)�[2m�[22m


Generated by 🚫 dangerJS

@pcvonz
Copy link
Contributor Author

pcvonz commented Aug 16, 2018

Updated, but the eslint fails are from the incorrect magento-eslint package.

- Switched import { Component, createElement } from 'react';) to import React from 'react', { Component };
- Switched import { Fragment, createElement } from 'react';) to import React from 'react', { Fragment };
- Switched import { createElement } from 'react';) to import React from 'react';
- Added `eslint-plugin-react` and updated eslint configuration
accordingly

Update eslintrc and remove react imports from components which have no jsx

Add Component import back to ContainerChild.js

chore: Update .babelrc and rm redundant files

Switch to correct React import

prettify! ❤️

chore: update eslint-config with pcvonz fisx
@zetlen zetlen self-requested a review September 18, 2018 23:19
zetlen
zetlen previously approved these changes Sep 18, 2018
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Hey @pcvonz, I updated eslint-config and fixed one stray pragma. I'm approving this for merge into the 1.x release, since it's such a large change!

@zetlen zetlen changed the base branch from master to release/1.0 September 25, 2018 20:40
@zetlen zetlen changed the base branch from release/1.0 to release/2.0 September 26, 2018 21:10
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's get this merged before too many other things go in.

@pcvonz Thanks for doing the legwork! I've merged the release branch into this and updated the components that didn't exist at the time you submitted this PR.

@zetlen zetlen merged commit 91aec39 into magento:release/2.0 Sep 26, 2018
@mage2pratik mage2pratik mentioned this pull request Jan 30, 2019
6 tasks
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.

5 participants