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

Rollup 0.59.x removes important code from ReactDOM bundle #2199

Closed
gaearon opened this issue May 20, 2018 · 4 comments
Closed

Rollup 0.59.x removes important code from ReactDOM bundle #2199

gaearon opened this issue May 20, 2018 · 4 comments

Comments

@gaearon
Copy link

gaearon commented May 20, 2018

Reproducing case:

  1. Clone https://github.com/isidrok/react-bug
  2. npm install
  3. npm start
  4. Open build.dev.js and scroll to diffProperties$1 function definition
  5. Open node_modules/react-dom/cjs/react-dom.development.js for comparison and scroll to diffProperties$1 function definition

You’ll see that the original function definition contains a for loop like

function diffProperties$1(domElement, tag, lastRawProps, nextRawProps, rootContainerElement) {
  // ...
  for (propKey in lastProps) {
    if (nextProps.hasOwnProperty(propKey) || !lastProps.hasOwnProperty(propKey) || lastProps[propKey] == null) {
      continue;
    }
    if (propKey === STYLE) {
      // ...
    } else if (propKey === DANGEROUSLY_SET_INNER_HTML || propKey === CHILDREN) {
      // ...
    } else if (registrationNameModules.hasOwnProperty(propKey)) {
      // ...
    } else {
      // ...
    }
    // ...
}

However the diffProperties$1 definition in the Rollup’d bundle.dev.js is completely missing the (essential) logic under those conditions as well as conditions themselves, except for the last one. This produces code that behaves differently. Its for loop only contains the registrationNameModules.hasOwnProperty(propKey) check and not the preceding ones.

I verified that downgrading to 0.58.x fixes it.

@isidrok
Copy link
Contributor

isidrok commented May 20, 2018

Didn't think about downgrading rollup since it was my first time trying out React 16.3... 😅

Seems it has something to do with treeshaking since you didn't change the version of rollup-plugin-replace

Thank you @gaearon !

@lukastaegert
Copy link
Member

Thanks a lot for posting this issue + the repro. It took me a while to figure out but it seems this is actually a rather old issue that has come to the surface with the enhanced conditional tracking. Basically it appears that for..in statements will not count as assignments to their loop variable.

This is a minimal repro which is completely removed:

let x = false;

for (x in {key: true}) {
	if (x) console.log('removed');
}

I think it should not be too difficult to fix, will take a look soon.

@lukastaegert
Copy link
Member

Fixed in Rollup v0.59.2, please check it out!

@isidrok
Copy link
Contributor

isidrok commented May 23, 2018

Hi, the reproduction demo works fine with last rollup version, thanks!

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

No branches or pull requests

3 participants