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

With transform-react-constant-elements enabled, having any logic with multiple-paths for return statements aren't caught as "ignore this one" #7200

Closed
nuclearspike opened this Issue Jan 11, 2018 · 33 comments

Comments

Projects
None yet
6 participants
@nuclearspike

nuclearspike commented Jan 11, 2018

I will often have functions like (over simplified):

const SomeThing = ({value}) => {
    if (typeof value === 'undefined') {
       return <div />
    } else {
       return <SomeComponent value={value} />
}

In previous versions of this plugin, it was apparently smart enough to recognize multiple-return-paths as being something where it couldn't apply this plugin and left it. However, this is not the case anymore and using this plugin caused all sorts of "Invariant Violation" errors in production that did not occur in dev. Disabling this plugin fixed production code.

I would recommend that if a const element has multiple return statements (obviously caused by logic) that it shouldn't convert that element. As of now, I had to disable the entire plugin where it may help 90%+ of my uses of constant elements but in the rare cases where it doesn't, it breaks in ways that give very cryptic errors and only in production ("Invariant violation" where the call stack was all react composite components, not indicating what element caused the issue) and since it works in dev, made it difficult to figure out this plugin was the cause. During my upgrade to newer react and babel components, it was not easy to recognize this plugin was the culprit.

Maybe having logic like above should only be used in classes, and if so, at least have this plugin spit out errors that can help users localize the issue ("const elements cannot have multiple return statements. See 'SomeThing' and convert it to a class, remove multiple return paths or disable the 'transform-react-constant-elements' plugin for production if you need that ability") would have saved me hours.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Jan 11, 2018

Collaborator

Hey @nuclearspike! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Jan 11, 2018

Hey @nuclearspike! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

I fail to see how that proposed solution would be good. Could you post the output code for your example?

Member

Andarist commented Jan 11, 2018

I fail to see how that proposed solution would be good. Could you post the output code for your example?

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

What do you see wrong with letting developers know they are using features that aren't supported?

nuclearspike commented Jan 11, 2018

What do you see wrong with letting developers know they are using features that aren't supported?

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

I'm guessing it could be because it's hoisting code that needs redux state to be processed already or something else. It's stuff that's happening behind the scenes where it assumes no consequence.

nuclearspike commented Jan 11, 2018

I'm guessing it could be because it's hoisting code that needs redux state to be processed already or something else. It's stuff that's happening behind the scenes where it assumes no consequence.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

What do you see wrong with letting developers know they are using features that aren't supported?

Could you explain? Im not sure if I understand what do you mean.

I'm guessing it could be because it's hoisting code that needs redux state to be processed already or something else. It's stuff that's happening behind the scenes where it assumes no consequence.

There might be a bug in our codebase, but to judge this it would be best to share input code + output of the transforms.

I dont agree with this statement:

I would recommend that if a const element has multiple return statements (obviously caused by logic) that it shouldn't convert that element.

Multiple return functions shouldnt be considered special. The only thing that matter is detecting if a certain element can be hoisted, no matter where it is in the code. We need to check your input and investigate what causes a detection to fail in your case.

Member

Andarist commented Jan 11, 2018

What do you see wrong with letting developers know they are using features that aren't supported?

Could you explain? Im not sure if I understand what do you mean.

I'm guessing it could be because it's hoisting code that needs redux state to be processed already or something else. It's stuff that's happening behind the scenes where it assumes no consequence.

There might be a bug in our codebase, but to judge this it would be best to share input code + output of the transforms.

I dont agree with this statement:

I would recommend that if a const element has multiple return statements (obviously caused by logic) that it shouldn't convert that element.

Multiple return functions shouldnt be considered special. The only thing that matter is detecting if a certain element can be hoisted, no matter where it is in the code. We need to check your input and investigate what causes a detection to fail in your case.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

My solutions were simplistic, I definitely agree. I would absolutely prefer it knowing if an element could be hoisted or not. I'd found that with my rather complicated code, that to turn off the plugin saved me tons of time in trying to debug every possible explanation of why my dev code works locally but fails in stage/production.

I'm not sure I know how to come up with up a test case where it fails if it's good at knowing what return paths are hoistable and what aren't without extensive testing if it's supposed to be able to handle really complex code . I didn't expect that level of analysis is being done.

Regarding usable info for devs, I just mean that all I got was "invariant violation" errors with call stacks that were all react composite code files that gave no indication as to what code of mine caused the problem. Anything, warnings during compilation, etc, would have been helpful, if it saw that I was doing things that might cause problems by it not being a class rather than a function and how I used it. Anything would have helped. Right now, I just have a "do not use! it'll cause tons of untraceable errors" comment surrounding the "transform-react-constant-elements" reference in webpack plugins section.

nuclearspike commented Jan 11, 2018

My solutions were simplistic, I definitely agree. I would absolutely prefer it knowing if an element could be hoisted or not. I'd found that with my rather complicated code, that to turn off the plugin saved me tons of time in trying to debug every possible explanation of why my dev code works locally but fails in stage/production.

I'm not sure I know how to come up with up a test case where it fails if it's good at knowing what return paths are hoistable and what aren't without extensive testing if it's supposed to be able to handle really complex code . I didn't expect that level of analysis is being done.

Regarding usable info for devs, I just mean that all I got was "invariant violation" errors with call stacks that were all react composite code files that gave no indication as to what code of mine caused the problem. Anything, warnings during compilation, etc, would have been helpful, if it saw that I was doing things that might cause problems by it not being a class rather than a function and how I used it. Anything would have helped. Right now, I just have a "do not use! it'll cause tons of untraceable errors" comment surrounding the "transform-react-constant-elements" reference in webpack plugins section.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

I'm not sure I know how to come up with up a test case where it fails if it's good at knowing what return paths are hoistable and what aren't without extensive testing if it's supposed to be able to handle really complex code . I didn't expect that level of analysis is being done.

Well, actually I would suspect that rules for this hoisting are not that complex - simplifying: it just cannot reference things from the closure to be hoistable.

Which version of babel do you use? 6 or 7? It would be great if you could provide an example code that caused those problems, even if its complex - we'll be able to trim it down.

Member

Andarist commented Jan 11, 2018

I'm not sure I know how to come up with up a test case where it fails if it's good at knowing what return paths are hoistable and what aren't without extensive testing if it's supposed to be able to handle really complex code . I didn't expect that level of analysis is being done.

Well, actually I would suspect that rules for this hoisting are not that complex - simplifying: it just cannot reference things from the closure to be hoistable.

Which version of babel do you use? 6 or 7? It would be great if you could provide an example code that caused those problems, even if its complex - we'll be able to trim it down.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

"devDependencies": {
    "@babel/core": "7.0.0-beta.36",
    "@babel/node": "7.0.0-beta.36",
    "@babel/plugin-proposal-decorators": "7.0.0-beta.36",
    "@babel/plugin-transform-react-constant-elements": "7.0.0-beta.36",
    "@babel/plugin-transform-react-inline-elements": "7.0.0-beta.36",
    "@babel/preset-env": "7.0.0-beta.36",
    "@babel/preset-flow": "7.0.0-beta.36",
    "@babel/preset-react": "7.0.0-beta.36",
    "@babel/preset-stage-2": "7.0.0-beta.36",
    "assets-webpack-plugin": "^3.5.1",
    "autoprefixer": "^7.2.4",
    "babel-eslint": "^8.2.1",
    "babel-loader": "^7.1.2",
    "babel-plugin-relay": "^1.4.1",
    "babel-plugin-trace": "^1.0.0",
    "babel-plugin-transform-react-constant-elements": "^6.23.0",
    "babel-plugin-transform-react-inline-elements": "^6.22.0",
    "babel-plugin-transform-react-jsx-self": "^6.22.0",
    "babel-plugin-transform-react-jsx-source": "^6.22.0",
    "babel-plugin-transform-react-remove-prop-types": "^0.4.12",

I appreciate that you've been very attentive to this issue. That's not very normal in most projects so it is definitely not unnoticed. I have a massive project, and wouldn't know what parts to share with you that you'd find interesting that might contribute to the issue.

nuclearspike commented Jan 11, 2018

"devDependencies": {
    "@babel/core": "7.0.0-beta.36",
    "@babel/node": "7.0.0-beta.36",
    "@babel/plugin-proposal-decorators": "7.0.0-beta.36",
    "@babel/plugin-transform-react-constant-elements": "7.0.0-beta.36",
    "@babel/plugin-transform-react-inline-elements": "7.0.0-beta.36",
    "@babel/preset-env": "7.0.0-beta.36",
    "@babel/preset-flow": "7.0.0-beta.36",
    "@babel/preset-react": "7.0.0-beta.36",
    "@babel/preset-stage-2": "7.0.0-beta.36",
    "assets-webpack-plugin": "^3.5.1",
    "autoprefixer": "^7.2.4",
    "babel-eslint": "^8.2.1",
    "babel-loader": "^7.1.2",
    "babel-plugin-relay": "^1.4.1",
    "babel-plugin-trace": "^1.0.0",
    "babel-plugin-transform-react-constant-elements": "^6.23.0",
    "babel-plugin-transform-react-inline-elements": "^6.22.0",
    "babel-plugin-transform-react-jsx-self": "^6.22.0",
    "babel-plugin-transform-react-jsx-source": "^6.22.0",
    "babel-plugin-transform-react-remove-prop-types": "^0.4.12",

I appreciate that you've been very attentive to this issue. That's not very normal in most projects so it is definitely not unnoticed. I have a massive project, and wouldn't know what parts to share with you that you'd find interesting that might contribute to the issue.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

The faulty component would be enough. Your shouldnt have installed both babel@7 and babel@6 deps together (community packages sometimes might be used for both versions) - could you remove those?

  • "babel-plugin-transform-react-constant-elements": "^6.23.0",
  • "babel-plugin-transform-react-inline-elements": "^6.22.0",
  • "babel-plugin-transform-react-jsx-self": "^6.22.0",
  • "babel-plugin-transform-react-jsx-source": "^6.22.0",

In this case sharing your .babelrc could be helpful too.

While we do our best to keep this bug-free, its still beta stage of babel@7 and possibility for bugs is a little bit higher than usual.

Member

Andarist commented Jan 11, 2018

The faulty component would be enough. Your shouldnt have installed both babel@7 and babel@6 deps together (community packages sometimes might be used for both versions) - could you remove those?

  • "babel-plugin-transform-react-constant-elements": "^6.23.0",
  • "babel-plugin-transform-react-inline-elements": "^6.22.0",
  • "babel-plugin-transform-react-jsx-self": "^6.22.0",
  • "babel-plugin-transform-react-jsx-source": "^6.22.0",

In this case sharing your .babelrc could be helpful too.

While we do our best to keep this bug-free, its still beta stage of babel@7 and possibility for bugs is a little bit higher than usual.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

I've removed those mentioned. However,

plugins: [
            ['relay', {
              schema: './graphQLSchema.json',
            }],
            // Treat React JSX elements as value types and hoist them to the highest scope
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-constant-elements
            // todo: LEAVE OUT!!!  ...(isDebug ? [] : ['@babel/transform-react-constant-elements']),
            // Replaces the React.createElement function with one that is more optimized for production
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-inline-elements
            ...(isDebug ? [] : ['@babel/transform-react-inline-elements']),
            // Remove unnecessary React propTypes from the production build
            // https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types
            ...(isDebug ? [] : ['transform-react-remove-prop-types']),
            '@babel/plugin-proposal-decorators',
          ],

From my assumption, since I wasn't directly referencing the plugins mentions, they should have not had effect.

as far as I understand it, my webpack.config.js says:

babelrc: false,

so it shouldn't read the .babelrc file; regardless:

My .babelrc


/**
 * React Starter Kit (https://www.reactstarterkit.com/)
 *
 * Copyright © 2014-present Kriasoft, LLC. All rights reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE.txt file in the root directory of this source tree.
 */

// Babel configuration
// https://babeljs.io/docs/usage/api/
module.exports = {
  plugins: [
    "relay",
  ],
  presets: [
    [
      '@babel/preset-env',
      {
        targets: {
          node: 'current',
        },
      },
    ],
    '@babel/preset-stage-2',
    '@babel/preset-flow',
    '@babel/preset-react',
  ],
  ignore: ['node_modules', 'build'],
};

nuclearspike commented Jan 11, 2018

I've removed those mentioned. However,

plugins: [
            ['relay', {
              schema: './graphQLSchema.json',
            }],
            // Treat React JSX elements as value types and hoist them to the highest scope
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-constant-elements
            // todo: LEAVE OUT!!!  ...(isDebug ? [] : ['@babel/transform-react-constant-elements']),
            // Replaces the React.createElement function with one that is more optimized for production
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-inline-elements
            ...(isDebug ? [] : ['@babel/transform-react-inline-elements']),
            // Remove unnecessary React propTypes from the production build
            // https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types
            ...(isDebug ? [] : ['transform-react-remove-prop-types']),
            '@babel/plugin-proposal-decorators',
          ],

From my assumption, since I wasn't directly referencing the plugins mentions, they should have not had effect.

as far as I understand it, my webpack.config.js says:

babelrc: false,

so it shouldn't read the .babelrc file; regardless:

My .babelrc


/**
 * React Starter Kit (https://www.reactstarterkit.com/)
 *
 * Copyright © 2014-present Kriasoft, LLC. All rights reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE.txt file in the root directory of this source tree.
 */

// Babel configuration
// https://babeljs.io/docs/usage/api/
module.exports = {
  plugins: [
    "relay",
  ],
  presets: [
    [
      '@babel/preset-env',
      {
        targets: {
          node: 'current',
        },
      },
    ],
    '@babel/preset-stage-2',
    '@babel/preset-flow',
    '@babel/preset-react',
  ],
  ignore: ['node_modules', 'build'],
};
@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

Oh, yeah - if you are using webpack-loader with this option babelrc is not what we are interested in, but rather options that you pass to the loader itself.

Member

Andarist commented Jan 11, 2018

Oh, yeah - if you are using webpack-loader with this option babelrc is not what we are interested in, but rather options that you pass to the loader itself.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

   rules: [
      // Rules for JS / JSX
      {
        test: reScript,
        include: [
          path.resolve(__dirname, '../src'),
          path.resolve(__dirname, '../tools'),
        ],
        loader: 'babel-loader',
        options: {
          // https://github.com/babel/babel-loader#options
          cacheDirectory: isDebug,

          // https://babeljs.io/docs/usage/options/
          babelrc: false,
          presets: [
            // A Babel preset that can automatically determine the Babel plugins and polyfills
            // https://github.com/babel/babel-preset-env
            [
              '@babel/preset-env',
              {
                targets: {
                  browsers: pkg.browserslist,
                  forceAllTransforms: !isDebug, // for UglifyJS
                },
                modules: false,
                useBuiltIns: false,
                debug: false,
              },
            ],
            // Experimental ECMAScript proposals
            // https://babeljs.io/docs/plugins/#presets-stage-x-experimental-presets-
            '@babel/preset-stage-2',
            // Flow
            // https://github.com/babel/babel/tree/master/packages/babel-preset-flow
            '@babel/preset-flow',
            // JSX
            // https://github.com/babel/babel/tree/master/packages/babel-preset-react
            ['@babel/preset-react', { development: isDebug }],
          ],
          plugins: [
            ['relay', {
              schema: './graphQLSchema.json',
            }],
            // Treat React JSX elements as value types and hoist them to the highest scope
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-constant-elements
            // todo: LEAVE OUT!!! ...(isDebug ? [] : ['@babel/transform-react-constant-elements']),
            // Replaces the React.createElement function with one that is more optimized for production
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-inline-elements
            ...(isDebug ? [] : ['@babel/transform-react-inline-elements']),
            // Remove unnecessary React propTypes from the production build
            // https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types
            ...(isDebug ? [] : ['transform-react-remove-prop-types']),
            '@babel/plugin-proposal-decorators',
          ],
        },
      },

Outside of this?

nuclearspike commented Jan 11, 2018

   rules: [
      // Rules for JS / JSX
      {
        test: reScript,
        include: [
          path.resolve(__dirname, '../src'),
          path.resolve(__dirname, '../tools'),
        ],
        loader: 'babel-loader',
        options: {
          // https://github.com/babel/babel-loader#options
          cacheDirectory: isDebug,

          // https://babeljs.io/docs/usage/options/
          babelrc: false,
          presets: [
            // A Babel preset that can automatically determine the Babel plugins and polyfills
            // https://github.com/babel/babel-preset-env
            [
              '@babel/preset-env',
              {
                targets: {
                  browsers: pkg.browserslist,
                  forceAllTransforms: !isDebug, // for UglifyJS
                },
                modules: false,
                useBuiltIns: false,
                debug: false,
              },
            ],
            // Experimental ECMAScript proposals
            // https://babeljs.io/docs/plugins/#presets-stage-x-experimental-presets-
            '@babel/preset-stage-2',
            // Flow
            // https://github.com/babel/babel/tree/master/packages/babel-preset-flow
            '@babel/preset-flow',
            // JSX
            // https://github.com/babel/babel/tree/master/packages/babel-preset-react
            ['@babel/preset-react', { development: isDebug }],
          ],
          plugins: [
            ['relay', {
              schema: './graphQLSchema.json',
            }],
            // Treat React JSX elements as value types and hoist them to the highest scope
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-constant-elements
            // todo: LEAVE OUT!!! ...(isDebug ? [] : ['@babel/transform-react-constant-elements']),
            // Replaces the React.createElement function with one that is more optimized for production
            // https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-react-inline-elements
            ...(isDebug ? [] : ['@babel/transform-react-inline-elements']),
            // Remove unnecessary React propTypes from the production build
            // https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types
            ...(isDebug ? [] : ['transform-react-remove-prop-types']),
            '@babel/plugin-proposal-decorators',
          ],
        },
      },

Outside of this?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

Ok, this config seems quite OK. Would still love to get a single faulty component's code for testing this. Are you able to provide it?

Member

Andarist commented Jan 11, 2018

Ok, this config seems quite OK. Would still love to get a single faulty component's code for testing this. Are you able to provide it?

@existentialism

This comment has been minimized.

Show comment
Hide comment
@existentialism

existentialism Jan 11, 2018

Member

@nuclearspike small sidenote, but @babel/preset-env actually reads from browserslist config sources (like package.json's browserslist key) by default.

Member

existentialism commented Jan 11, 2018

@nuclearspike small sidenote, but @babel/preset-env actually reads from browserslist config sources (like package.json's browserslist key) by default.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

So, knowing which component it was is part of the issue. I only know that shutting off the plugin solved it. I don't know which component(s) were the problem, one of the pages where it broke has this which looks a little like it might cause issues even though it's not the multiple return path issue (which was just an assumption of mine based on the examples I saw in the docs of what the plugin was doing):

const PanelBlock = ({ ListComponent, list, title, description }) => {
  const hasList = list && list.length > 0;
  return (
    <Col sm={6}>
      <Panel header={title} bsStyle={hasList ? 'warning' : 'success'}>
        {description}
        { hasList ? <ListComponent list={list} /> : <div>None</div>}
      </Panel>
    </Col>
  );
};
PanelBlock.propTypes = {
  ListComponent: PropTypes.func,
  list: PropTypes.arrayOf(PropTypes.object),
  title: PropTypes.string,
  description: PropTypes.string,
}

Where the usage looks like:

       <PanelBlock
          title="Draft Providers"
          description="Either complete these drafts and publish them, or reject them."
          ListComponent={ProviderList}
          list={overview.providers.drafts}
        />

And ProviderList is:

const ProviderList = ({ list }) => (
  <ul>
    {list.map(provider => (<li key={provider.slug}>
      <a href={`#/manage/providers/${provider.slug}`}>
        {provider.name}
      </a>
       </li>))}
  </ul>);
ProviderList.propTypes = {
  list: PropTypes.arrayOf(PropTypes.shape({
    slug: PropTypes.string,
    name: PropTypes.string,
  })),
}

nuclearspike commented Jan 11, 2018

So, knowing which component it was is part of the issue. I only know that shutting off the plugin solved it. I don't know which component(s) were the problem, one of the pages where it broke has this which looks a little like it might cause issues even though it's not the multiple return path issue (which was just an assumption of mine based on the examples I saw in the docs of what the plugin was doing):

const PanelBlock = ({ ListComponent, list, title, description }) => {
  const hasList = list && list.length > 0;
  return (
    <Col sm={6}>
      <Panel header={title} bsStyle={hasList ? 'warning' : 'success'}>
        {description}
        { hasList ? <ListComponent list={list} /> : <div>None</div>}
      </Panel>
    </Col>
  );
};
PanelBlock.propTypes = {
  ListComponent: PropTypes.func,
  list: PropTypes.arrayOf(PropTypes.object),
  title: PropTypes.string,
  description: PropTypes.string,
}

Where the usage looks like:

       <PanelBlock
          title="Draft Providers"
          description="Either complete these drafts and publish them, or reject them."
          ListComponent={ProviderList}
          list={overview.providers.drafts}
        />

And ProviderList is:

const ProviderList = ({ list }) => (
  <ul>
    {list.map(provider => (<li key={provider.slug}>
      <a href={`#/manage/providers/${provider.slug}`}>
        {provider.name}
      </a>
       </li>))}
  </ul>);
ProviderList.propTypes = {
  list: PropTypes.arrayOf(PropTypes.shape({
    slug: PropTypes.string,
    name: PropTypes.string,
  })),
}
@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 11, 2018

But, I don't know that those are the issue. I'd even set up that page to not do any server render other than a spinner (and do everything in browser only) and it would still give errors at the server... so it's possible that just having components like what in the code-split package and hoisted may cause the problem?

nuclearspike commented Jan 11, 2018

But, I don't know that those are the issue. I'd even set up that page to not do any server render other than a spinner (and do everything in browser only) and it would still give errors at the server... so it's possible that just having components like what in the code-split package and hoisted may cause the problem?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 12, 2018

Member

so it's possible that just having components like what in the code-split package and hoisted may cause the problem?

That is unlikely.

I've checked code that you have pasted in, but this one seems to be transformed correctly. We'd love to fix this asap, but without actual repro case we cannot do much 😢

Member

Andarist commented Jan 12, 2018

so it's possible that just having components like what in the code-split package and hoisted may cause the problem?

That is unlikely.

I've checked code that you have pasted in, but this one seems to be transformed correctly. We'd love to fix this asap, but without actual repro case we cannot do much 😢

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 12, 2018

Hmm. I'm not sure how I can debug this to tell you what component it is... that was actually a big part of the problem. No matter what I did to the component, the error was still happening. I eventually even wrote this decorator:

export function noServerRender() {
  return ComposedComponent => class ClientOnly extends Component {
    constructor(props) {
      super(props);
      this.state = { onClient: false };
    }

    componentDidMount() {
      setImmediate(() => this.setState({ onClient: true }))
    }

    render() {
      if (!this.state.onClient) {
        return <Loading />
      }
      return <ComposedComponent {...this.props} />
    }
  }
}

to try to narrow things down. So, for the server-side render of this, it should only render <Loading /> and only after the package loads and renders once in the client will it try to load anything else. The invariant errors were happening on the server. It must have happened as soon as the 'admin' package was loaded into memory in the server and then other pages that had previously worked would stop working. For me, that hints at some code is executing during package load due to this plugin that causes the problems since hitting the admin page, the server errored, with only one simple Loading spinner rendering.

Is it possible that the decorator plugin should be placed before this one?

nuclearspike commented Jan 12, 2018

Hmm. I'm not sure how I can debug this to tell you what component it is... that was actually a big part of the problem. No matter what I did to the component, the error was still happening. I eventually even wrote this decorator:

export function noServerRender() {
  return ComposedComponent => class ClientOnly extends Component {
    constructor(props) {
      super(props);
      this.state = { onClient: false };
    }

    componentDidMount() {
      setImmediate(() => this.setState({ onClient: true }))
    }

    render() {
      if (!this.state.onClient) {
        return <Loading />
      }
      return <ComposedComponent {...this.props} />
    }
  }
}

to try to narrow things down. So, for the server-side render of this, it should only render <Loading /> and only after the package loads and renders once in the client will it try to load anything else. The invariant errors were happening on the server. It must have happened as soon as the 'admin' package was loaded into memory in the server and then other pages that had previously worked would stop working. For me, that hints at some code is executing during package load due to this plugin that causes the problems since hitting the admin page, the server errored, with only one simple Loading spinner rendering.

Is it possible that the decorator plugin should be placed before this one?

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 12, 2018

Just tested it, and ordering the decorator plugin first made no difference. I've never included the full stack trace if it helps at all.

Minified React error #130; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.

Invariant Violation: Minified React error #130; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
    at reactProdInvariant (/app/node_modules/react-dom/lib/reactProdInvariant.js:29:15)
    at instantiateReactComponent (/app/node_modules/react-dom/lib/instantiateReactComponent.js:72:250)
    at instantiateChild (/app/node_modules/react-dom/lib/ReactChildReconciler.js:42:28)
    at traverseAllChildrenImpl (/app/node_modules/react-dom/lib/traverseAllChildren.js:75:5)
    at traverseAllChildren (/app/node_modules/react-dom/lib/traverseAllChildren.js:170:10)
    at Object.instantiateChildren (/app/node_modules/react-dom/lib/ReactChildReconciler.js:72:7)
    at ReactDOMComponent._reconcilerInstantiateChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:189:35)
    at ReactDOMComponent.mountChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:222:27)
    at ReactDOMComponent._createContentMarkup (/app/node_modules/react-dom/lib/ReactDOMComponent.js:657:32)
    at ReactDOMComponent.mountComponent (/app/node_modules/react-dom/lib/ReactDOMComponent.js:524:29)
    at Object.mountComponent (/app/node_modules/react-dom/lib/ReactReconciler.js:43:35)
    at ReactCompositeComponentWrapper.performInitialMount (/app/node_modules/react-dom/lib/ReactCompositeComponent.js:368:34)
    at ReactCompositeComponentWrapper.mountComponent (/app/node_modules/react-dom/lib/ReactCompositeComponent.js:255:21)
    at Object.mountComponent (/app/node_modules/react-dom/lib/ReactReconciler.js:43:35)
    at ReactDOMComponent.mountChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:234:44)
    at ReactDOMComponent._createContentMarkup (/app/node_modules/react-dom/lib/ReactDOMComponent.js:657:32)

nuclearspike commented Jan 12, 2018

Just tested it, and ordering the decorator plugin first made no difference. I've never included the full stack trace if it helps at all.

Minified React error #130; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.

Invariant Violation: Minified React error #130; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
    at reactProdInvariant (/app/node_modules/react-dom/lib/reactProdInvariant.js:29:15)
    at instantiateReactComponent (/app/node_modules/react-dom/lib/instantiateReactComponent.js:72:250)
    at instantiateChild (/app/node_modules/react-dom/lib/ReactChildReconciler.js:42:28)
    at traverseAllChildrenImpl (/app/node_modules/react-dom/lib/traverseAllChildren.js:75:5)
    at traverseAllChildren (/app/node_modules/react-dom/lib/traverseAllChildren.js:170:10)
    at Object.instantiateChildren (/app/node_modules/react-dom/lib/ReactChildReconciler.js:72:7)
    at ReactDOMComponent._reconcilerInstantiateChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:189:35)
    at ReactDOMComponent.mountChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:222:27)
    at ReactDOMComponent._createContentMarkup (/app/node_modules/react-dom/lib/ReactDOMComponent.js:657:32)
    at ReactDOMComponent.mountComponent (/app/node_modules/react-dom/lib/ReactDOMComponent.js:524:29)
    at Object.mountComponent (/app/node_modules/react-dom/lib/ReactReconciler.js:43:35)
    at ReactCompositeComponentWrapper.performInitialMount (/app/node_modules/react-dom/lib/ReactCompositeComponent.js:368:34)
    at ReactCompositeComponentWrapper.mountComponent (/app/node_modules/react-dom/lib/ReactCompositeComponent.js:255:21)
    at Object.mountComponent (/app/node_modules/react-dom/lib/ReactReconciler.js:43:35)
    at ReactDOMComponent.mountChildren (/app/node_modules/react-dom/lib/ReactMultiChild.js:234:44)
    at ReactDOMComponent._createContentMarkup (/app/node_modules/react-dom/lib/ReactDOMComponent.js:657:32)
@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 13, 2018

If you'd like access to my github repo to go hunting for possible issues, I am willing to grant that.

nuclearspike commented Jan 13, 2018

If you'd like access to my github repo to go hunting for possible issues, I am willing to grant that.

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 13, 2018

@existentialism You may want to make that comment on https://github.com/kriasoft/react-starter-kit 's project since that's where that code is from. For me, you've got an audience of one, but for them you're a babel member commenting on their code which has 16K watches. It would have much greater reach and more likely chance to change thousands of projects rather than just mine. Thanks for the comment, btw. You may want to direct it upstream though.

nuclearspike commented Jan 13, 2018

@existentialism You may want to make that comment on https://github.com/kriasoft/react-starter-kit 's project since that's where that code is from. For me, you've got an audience of one, but for them you're a babel member commenting on their code which has 16K watches. It would have much greater reach and more likely chance to change thousands of projects rather than just mine. Thanks for the comment, btw. You may want to direct it upstream though.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 13, 2018

Member

I'd be willing to go hunt for this bug, if you grant me the access to your repo. Was this issue happening often in prod mode? Or only rarely?

Member

Andarist commented Jan 13, 2018

I'd be willing to go hunt for this bug, if you grant me the access to your repo. Was this issue happening often in prod mode? Or only rarely?

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 13, 2018

It happened every time I hit the /admin page. The error used to also happen also during /login or /register but those I discovered were solved by null references for classnames with imports. In dev, className={s.someClass} would not throw an error but if .someClass didn't exist in the import s from './SomeClass.scss' it also threw invariant errors. Dunno if that's a hint. I'll add you as a collaborator to my repo. Just don't go starting a travel company using the code :)

nuclearspike commented Jan 13, 2018

It happened every time I hit the /admin page. The error used to also happen also during /login or /register but those I discovered were solved by null references for classnames with imports. In dev, className={s.someClass} would not throw an error but if .someClass didn't exist in the import s from './SomeClass.scss' it also threw invariant errors. Dunno if that's a hint. I'll add you as a collaborator to my repo. Just don't go starting a travel company using the code :)

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 13, 2018

Member

Thanks, gonna try to take a look at it this weekend.

Just don't go starting a travel company using the code :)

I certainly won't ;)

Member

Andarist commented Jan 13, 2018

Thanks, gonna try to take a look at it this weekend.

Just don't go starting a travel company using the code :)

I certainly won't ;)

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 15, 2018

Member

Ok, I got it. It's actually a webpack bug. You'd have to file it there. I think it's because your quite "specific" directory structure. Let's look at ./src/components/Common/Decorators.jsx .

It has import { Loading } from '../Common'; when we are actually in the Common directory already (but that doesnt matter I think, I've also tried import { Loading } from '../Smarty'; and it was bugged the same way). I think this import redirects us to the package.json in this directory which has main field with ./Common.jsx. So we visit this file and it has such statements:

import { Loading } from '../Smarty'
export { Loading, DataLoader, Page };

Where Smarty is a similar directory with package.json and stuff (I dont think you need those package.jsons - whats ur use for them?).

When we inspect babel's output for this original file we won't see anything odd, stripping irrelevant stuff it got transpiled to:

import { Loading } from '../Common'; 

var _ref2 = React.createElement(Loading, {
  __source: {
    fileName: _jsxFileName,
    lineNumber: 37
  },
  __self: this
});

// ... in render
if (!ready) {
  return _ref2;
}

So far so good. But webpack's output for the hoisted contant element will be this:

var _ref4 = __WEBPACK_IMPORTED_MODULE_0_react___default.a.createElement(__WEBPACK_IMPORTED_MODULE_5__Common__["g" /* Loading */], {
  __source: {
    fileName: _jsxFileName,
    lineNumber: 218
  },
  __self: this
});

We can see that the Loading got transformed to an object containing the module where each its export is available as getter on the object. During the initialization ALL those getters from this object result in undefined (I suppose the requested module has not yet registered itself in webpack's runtime). After the initialization this object is already populated and that's why it works without the contant-elements transform - because those imports are requested later, when they are already available.

Member

Andarist commented Jan 15, 2018

Ok, I got it. It's actually a webpack bug. You'd have to file it there. I think it's because your quite "specific" directory structure. Let's look at ./src/components/Common/Decorators.jsx .

It has import { Loading } from '../Common'; when we are actually in the Common directory already (but that doesnt matter I think, I've also tried import { Loading } from '../Smarty'; and it was bugged the same way). I think this import redirects us to the package.json in this directory which has main field with ./Common.jsx. So we visit this file and it has such statements:

import { Loading } from '../Smarty'
export { Loading, DataLoader, Page };

Where Smarty is a similar directory with package.json and stuff (I dont think you need those package.jsons - whats ur use for them?).

When we inspect babel's output for this original file we won't see anything odd, stripping irrelevant stuff it got transpiled to:

import { Loading } from '../Common'; 

var _ref2 = React.createElement(Loading, {
  __source: {
    fileName: _jsxFileName,
    lineNumber: 37
  },
  __self: this
});

// ... in render
if (!ready) {
  return _ref2;
}

So far so good. But webpack's output for the hoisted contant element will be this:

var _ref4 = __WEBPACK_IMPORTED_MODULE_0_react___default.a.createElement(__WEBPACK_IMPORTED_MODULE_5__Common__["g" /* Loading */], {
  __source: {
    fileName: _jsxFileName,
    lineNumber: 218
  },
  __self: this
});

We can see that the Loading got transformed to an object containing the module where each its export is available as getter on the object. During the initialization ALL those getters from this object result in undefined (I suppose the requested module has not yet registered itself in webpack's runtime). After the initialization this object is already populated and that's why it works without the contant-elements transform - because those imports are requested later, when they are already available.

@Andarist Andarist closed this Jan 15, 2018

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Jan 23, 2018

I really appreciate how deeply you looked into the issue I was having. 👍

nuclearspike commented Jan 23, 2018

I really appreciate how deeply you looked into the issue I was having. 👍

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 23, 2018

Member

Thanks for having a trust with me and sharing the private repo, it was invaluable in finding the problem. It was quite bizarre and I would suspect babel being the culprit too - have you reported this to the webpack team?

Member

Andarist commented Jan 23, 2018

Thanks for having a trust with me and sharing the private repo, it was invaluable in finding the problem. It was quite bizarre and I would suspect babel being the culprit too - have you reported this to the webpack team?

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Mar 20, 2018

I'm pretty sure that I know what's the problem here.

This transformation:

// X.js
import { A } from "./module.js";
export const X = () => {
  return <A />;
};
// X.js
import { A } from "./module.js";
const _ref = <A />;

export const X = () => {
  return _ref;
};

is not always safe. In the original code, the A identifier is evaluated later. This is relevant in the case of circular dependencies (or when A is changing over time, it's a live binding, but that's rar).

i. e. it would break for this module:

// module.js
import { X } from "./X.js"

export const A = () => {
  // using X somehow, i. e. 
  return <div>{X.text}</div>
};

Everything works great without hoisting, because identifiers are accessed outside of the evaluation tick. But it breaks with hoisting constants.


You could transform it i. e. this way to fix it:

// X.js
import { A } from "./module.js";
const _ref = () => {
  const r = <A />;
  _ref = () => r;
  return r;
};

export const X = () => {
  return _ref();
};

This uses caching instead of hoisting. Maybe even better for faster bootstraping...

sokra commented Mar 20, 2018

I'm pretty sure that I know what's the problem here.

This transformation:

// X.js
import { A } from "./module.js";
export const X = () => {
  return <A />;
};
// X.js
import { A } from "./module.js";
const _ref = <A />;

export const X = () => {
  return _ref;
};

is not always safe. In the original code, the A identifier is evaluated later. This is relevant in the case of circular dependencies (or when A is changing over time, it's a live binding, but that's rar).

i. e. it would break for this module:

// module.js
import { X } from "./X.js"

export const A = () => {
  // using X somehow, i. e. 
  return <div>{X.text}</div>
};

Everything works great without hoisting, because identifiers are accessed outside of the evaluation tick. But it breaks with hoisting constants.


You could transform it i. e. this way to fix it:

// X.js
import { A } from "./module.js";
const _ref = () => {
  const r = <A />;
  _ref = () => r;
  return r;
};

export const X = () => {
  return _ref();
};

This uses caching instead of hoisting. Maybe even better for faster bootstraping...

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Mar 20, 2018

Member

@nuclearspike could u confirm that ur issue could have been caused by a circular dependency?

Member

Andarist commented Mar 20, 2018

@nuclearspike could u confirm that ur issue could have been caused by a circular dependency?

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Mar 21, 2018

I guess, you need to change @sokra 's example to use let 😁

// X.js
import { A } from "./module.js";
let _ref = () => {
  const r = <A />;
  _ref = () => r;
  return r;
};

export const X = () => {
  return _ref();
};

And I guess this is not only a problem with webpack, but would also be a problem with ESM and live bindings?

jhnns commented Mar 21, 2018

I guess, you need to change @sokra 's example to use let 😁

// X.js
import { A } from "./module.js";
let _ref = () => {
  const r = <A />;
  _ref = () => r;
  return r;
};

export const X = () => {
  return _ref();
};

And I guess this is not only a problem with webpack, but would also be a problem with ESM and live bindings?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Mar 21, 2018

Member

And I guess this is not only a problem with webpack, but would also be a problem with ESM and live bindings?

Yeah, gonna work on tweaking this. Although I'm curious if cyclic dep was really what caused it in this case. Need to investigate this

Member

Andarist commented Mar 21, 2018

And I guess this is not only a problem with webpack, but would also be a problem with ESM and live bindings?

Yeah, gonna work on tweaking this. Although I'm curious if cyclic dep was really what caused it in this case. Need to investigate this

@nuclearspike

This comment has been minimized.

Show comment
Hide comment
@nuclearspike

nuclearspike Mar 21, 2018

I did find a circular dependency in my Common.jsx file where it referenced Page.jsx which also had a reference to Common.jsx. Too many components in that file.

The conclusion we'd settled on was that the Loading component was the culprit, but I'd later changed it from a function component to a full class and the problem still existed, which was surprising... pointing at something else as the cause... so it being a circular reference issue may have been it.

nuclearspike commented Mar 21, 2018

I did find a circular dependency in my Common.jsx file where it referenced Page.jsx which also had a reference to Common.jsx. Too many components in that file.

The conclusion we'd settled on was that the Loading component was the culprit, but I'd later changed it from a function component to a full class and the problem still existed, which was surprising... pointing at something else as the cause... so it being a circular reference issue may have been it.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Mar 23, 2018

Member

@nuclearspike Thanks for checking and confirming that it could be this one.

Member

Andarist commented Mar 23, 2018

@nuclearspike Thanks for checking and confirming that it could be this one.

@lock lock bot added the outdated label Jun 22, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jun 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.