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

Master UMD builds don’t work as CommonJS using Webpack alias config #7482

Closed
gaearon opened this issue Aug 12, 2016 · 8 comments · Fixed by #7840
Closed

Master UMD builds don’t work as CommonJS using Webpack alias config #7482

gaearon opened this issue Aug 12, 2016 · 8 comments · Fixed by #7840
Assignees

Comments

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2016

Not sure what’s up yet but putting this up so I don’t forget to check it later.
In the past, I could put this into Webpack config:

    resolve: {
        alias: {
            react: 'react/dist/react',
            'react-dom': 'react-dom/dist/react-dom',
        },
      }

Many people do that to improve build times.

This works with 0.14.x and 15.3.0 but not in master.
Most likely related to recent @sebmarkbage changes.

It fails like this:

screen shot 2016-08-12 at 14 55 55

Seems like it claims to be a UMD shim but assumes global environment (whereas a true UMD shim would detect environment and fallback to require when available).

cc @zpao

@sebmarkbage
Copy link
Collaborator

Yea this came up in the PR. @zpao said it was broken anyway if used in CommonJS so I just built it to assume global.

We can add detection to UMDShim files.

@sebmarkbage
Copy link
Collaborator

If you could set up an example file or unit test that tests this, it would be very helpful to ensure no regressions.

@gaearon gaearon assigned gaearon and unassigned sebmarkbage Aug 12, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 12, 2016

Will do next week

@zpao
Copy link
Member

zpao commented Aug 12, 2016

@zpao said it was broken anyway if used in CommonJS

See #7092.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 12, 2016

To be clear, it was broken if you use just CJS react-dom build but it was not broken if you alias them both (Node can’t do that but Webpack/Browserify can).

@zpao
Copy link
Member

zpao commented Sep 30, 2016

I finally came back around to this and tried a few things. I hit a bit of a wall though, so not sure how to progress. We might need to do our own UMD bundle but that's going to get tedious. My main approach to try to solve this was to bring back the old UMD shims. Bundling seems to work fine BUT they still don't play nicely together using webpack. The main reason seems to be that Webpack is doing some detection if the alias is using require inside of it. Specifically the current UMD we ship for react-dom only has the single call of require and so that gets rewritten to __webpack_require__ but the full browserify bundle does not. So when the new react-dom bundle require('react') it doesn't get rewritten, so it doesn't look into webpack's module registry to find react, instead only looking in that dist-scoped registry.

I'll try to give this some more time tomorrow since I want to ship 15.4 but we might have to take more drastic measures. Maybe this can resolve itself if we use webpack to bundle our dist files? If we have to bite the bullet and do that, then so be it. Or if we have to write a wrapper around the bundle that browserify generates, we could try that too. Any thought's @gaearon?

@zpao
Copy link
Member

zpao commented Sep 30, 2016

I think this problem probably gets worse when we throw derequire and collapser back in the mix, though haven't tested to see how those play with declaring that 'react' is external...

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 30, 2016

Maybe this can resolve itself if we use webpack to bundle our dist files?

It might. (And in my opinion it would be simpler to do than having a bunch of post-processing plugins but I might be wrong 😄 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants