Skip to content

package.json updates#1159

Merged
justin808 merged 12 commits intomasterfrom
coopersamuel-package-updates
Oct 24, 2018
Merged

package.json updates#1159
justin808 merged 12 commits intomasterfrom
coopersamuel-package-updates

Conversation

@coopersamuel
Copy link
Copy Markdown
Contributor

@coopersamuel coopersamuel commented Oct 19, 2018

This PR updates most of the packages in package.json. I'll leave webpack for a separate PR because I anticipate breaking changes moving to v4.

More comments are in the code


This change is Reviewable

Comment thread .eslintrc Outdated
no-console: 0
function-paren-newline: 0
object-curly-newline: 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After updating eslint these two rules produced over 120 errors. I disabled them in the .eslintrc file


export function consoleReplay() {
// console.history is a global polyfill used in server rendering.
// $FlowFixMe
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As per this flow article, flow is now throwing errors for unknown property access in conditionals. I flagged this with $FlowFixMe for the time being to suppress the error. @justin808 what are your thoughts on fixing this?


export function clientStartup(context) {
const document = context.document;
const { document } = context;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reformatted to satiate eslint

if (reactElementOrRouterResult.redirectLocation) {
if (trace) {
const redirectLocation = reactElementOrRouterResult.redirectLocation;
const { redirectLocation } = reactElementOrRouterResult;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

More destructuring to make eslint happy


// eslint-disable-next-line no-underscore-dangle
const actual = ReactOnRails.render('R1', {}, 'root')._reactInternalInstance._currentElement.type;
const actual = ReactOnRails.render('R1', {}, 'root')._reactInternalFiber.type;
Copy link
Copy Markdown
Contributor Author

@coopersamuel coopersamuel Oct 19, 2018

Choose a reason for hiding this comment

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

When updating to React v16 there was a breaking change that caused the below test to fail

test('ReactOnRails render returns a virtual DOM element for component', (assert) => {
  assert.plan(1);
  const R1 = createReactClass({
    render() {
      return (
        <div> WORLD </div>
      );
    },
  });
  ReactOnRails.register({ R1 });
  // eslint-disable-next-line no-underscore-dangle
  const actual = ReactOnRails.render('R1', {}, 'root')._reactInternalInstance._currentElement.type;
  assert.deepEqual(actual, R1,
    'ReactOnRails render should return a virtual DOM element for component');
});

The error occurred because _reactInternalInstance was deprecated in React 16. The object returned from ReactDOM.render() for v15.6 looked like this (simplified):

{
    _reactInternalInstance: {
        _currentElement: {
            type: // Stuff here
        }
    }
}

After upgrading to React v16, the object looked like this (again, simplified):

{
    _reactInternalFiber: {
        type: // Stuff here
    },
    _reactInternalInstance: {
        // EMPTY!
    }
}

A one line update to the test above resulted in all tests passing
const actual = ReactOnRails.render('R1', {}, 'root')._reactInternalFiber.type;

arguments: [
'some message </script><script>alert(\'WTF\')</script>',
{ a: 'Wow</script><script>alert(\'WTF\')</script>', b: 2 },
],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reformatted for eslint

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 19, 2018

Coverage Status

Coverage remained the same at ?% when pulling 38d9c1c on coopersamuel-package-updates into 18e7840 on master.

Copy link
Copy Markdown
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Some changes still needed.

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @justin808 and @coopersamuel)


.eslintrc, line 20 at r1 (raw file):

function-paren-newline
  1. This will probably change once we put prettier in the project.
  2. We use: https://github.com/shakacode/style-guide-javascript/tree/master/packages/eslint-config-shakacode

node_package/src/buildConsoleReplay.js, line 8 at r1 (raw file):

Previously, coopersamuel (Samuel Cooper) wrote…

As per this flow article, flow is now throwing errors for unknown property access in conditionals. I flagged this with $FlowFixMe for the time being to suppress the error. @justin808 what are your thoughts on fixing this?

yeah, let's ignore for now.


spec/dummy/spec/system/integration_spec.rb, line 189 at r1 (raw file):

  scenario "deferring the initial render should prevent a client/server checksum mismatch error" do
    root = page.find(:xpath, "//div[@data-reactroot]")
    expect(root["data-react-checksum"].present?).to be(false)

This really doesn't show the test did anything.

We'll fix this.

@coopersamuel
Copy link
Copy Markdown
Contributor Author

@justin808 regarding your comments, what steps do you think need to be taken to pass this PR?

  1. Regarding the eslint rules, if we remove those rules we'd have a bunch of errors (> 120) - my position is that we leave the rules for now and add prettier in the project in a separate PR

  2. Regarding the test, I agree that test doesn't really do much, personally I think the test is now obsolete because of React's behavior removing the data-react-checksum attribute. Should we scrap the test entirely?

Let me know your thoughts on these and we'll get this pushed through

* fixed tests
* updated packages in spec/dummy
* add prettier
@justin808 justin808 force-pushed the coopersamuel-package-updates branch from 26442b9 to 1fec85a Compare October 22, 2018 08:10
Can't use match? on Ruby 2.2. Uncomment when Ruby 2.4 is
used for all test platforms
Copy link
Copy Markdown
Member

@justin808 justin808 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 add an entry to the CHANGELOG.md for adding hydrate

   * @param props Props to pass to your component
   * @param domNodeId
   * @param hydrate Pass truthy to update server rendered html. Default is falsy
   * @returns {virtualDomElement} Reference to your component's backing instance
   */

Reviewed 74 of 74 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @coopersamuel)


package-scripts.yml, line 44 at r2 (raw file):

        description: Check if any JSON files would change by running prettier-eslint.
        script: prettier "**/*.json" "../spec/**/*.json" --list-different
  renderer:

remove this


node_package/src/ReactOnRails.js, line 156 at r2 (raw file):

   * @param props Props to pass to your component
   * @param domNodeId
   * @param hydrate Pass truthy to update server rendered html. Default is falsy

add to CHANGELOG.md and doc

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.

3 participants