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

Issue with adding meterial-ui library #1210

Closed
wer32 opened this Issue Nov 7, 2016 · 14 comments

Comments

Projects
None yet
7 participants
@wer32

wer32 commented Nov 7, 2016

@samit4me I'm sorry for recreate the issue #1202 which your just closed. But I don't see the way how to fix issue in dev branch also. I followed all your instruction but the issue is still here.

I found another way to reproduce issue. You can
npm run start:production
and when you run code with your changes it show exception below in browser console.

Uncaught TypeError: Cannot read property 'document' of undefined
    at Object../ (http://localhost:8080/main.b2108cb64429ca259424.js:1:16840)
    at Object.<anonymous> (http://localhost:8080/main.b2108cb64429ca259424.js:1:1892)
    at t (http://localhost:8080/main.b2108cb64429ca259424.js:1:101)
    at Object../ (http://localhost:8080/main.b2108cb64429ca259424.js:1:16836)
    at Object.<anonymous> (http://localhost:8080/main.b2108cb64429ca259424.js:1:1892)
    at t (http://localhost:8080/main.b2108cb64429ca259424.js:1:101)
    at Object../node_modules/core-js/modules/_export.js (http://localhost:8080/main.b2108cb64429ca259424.js:8:5822)
    at t (http://localhost:8080/main.b2108cb64429ca259424.js:1:101)
    at Object../ (http://localhost:8080/main.b2108cb64429ca259424.js:1:16836)
    at Object.<anonymous> (http://localhost:8080/main.b2108cb64429ca259424.js:1:1892)

I've attached patch for you to be sure that I correct follow your instruction.
_1202_issue.zip

@wer32

This comment has been minimized.

Show comment
Hide comment
@wer32

wer32 Nov 7, 2016

If remove next lines from internals/webpack/webpack.prod.babel.js

    // Merge all duplicate modules
    new webpack.optimize.DedupePlugin(),

Then material-ui library start work correct.

wer32 commented Nov 7, 2016

If remove next lines from internals/webpack/webpack.prod.babel.js

    // Merge all duplicate modules
    new webpack.optimize.DedupePlugin(),

Then material-ui library start work correct.

@samit4me samit4me added the question label Nov 8, 2016

@samit4me

This comment has been minimized.

Show comment
Hide comment
@samit4me

samit4me Nov 8, 2016

Contributor

@wer32 I'm sorry about that. When I retested in the dev branch, I totally forgot about it ONLY being a production problem only. Then when I read Hi, this is what works for me: followed by essentially a repeat of what I said in my previous comment, I assumed it was all good. I will try and have another look later.

Contributor

samit4me commented Nov 8, 2016

@wer32 I'm sorry about that. When I retested in the dev branch, I totally forgot about it ONLY being a production problem only. Then when I read Hi, this is what works for me: followed by essentially a repeat of what I said in my previous comment, I assumed it was all good. I will try and have another look later.

@samit4me

This comment has been minimized.

Show comment
Hide comment
@samit4me

samit4me Nov 8, 2016

Contributor

So after riding my motorbike home in a storm 🌩🌧🌈 I did some more testing and the production build failed to work on the dev branch, sorry about that.

I tested it on that other project I mentioned earlier, which uses the webpack.optimize.DedupePlugin() and it works, BUT it is still on webpack v1.

After removing the webpack.optimize.DedupePlugin() as you suggested it works perfectly fine, nice work 👌. So I went looking for known issues, but there is so many issues logged against DedupePlugin() I gave up looking.

The webpack.optimize.DedupePlugin() appears to have very little effect on filesize anyways (see diff below).

dedupeplugindiff

I vote that we remove it, but I'm no webpack expert by any stretch of the imagination, so let's see what others think? @mxstbr

About a week ago the other optimization plugins webpack.optimize.OccurrenceOrderPlugin and webpack.optimize.UglifyJsPlugin were removed in this update webpack commit for issue #1032.

Contributor

samit4me commented Nov 8, 2016

So after riding my motorbike home in a storm 🌩🌧🌈 I did some more testing and the production build failed to work on the dev branch, sorry about that.

I tested it on that other project I mentioned earlier, which uses the webpack.optimize.DedupePlugin() and it works, BUT it is still on webpack v1.

After removing the webpack.optimize.DedupePlugin() as you suggested it works perfectly fine, nice work 👌. So I went looking for known issues, but there is so many issues logged against DedupePlugin() I gave up looking.

The webpack.optimize.DedupePlugin() appears to have very little effect on filesize anyways (see diff below).

dedupeplugindiff

I vote that we remove it, but I'm no webpack expert by any stretch of the imagination, so let's see what others think? @mxstbr

About a week ago the other optimization plugins webpack.optimize.OccurrenceOrderPlugin and webpack.optimize.UglifyJsPlugin were removed in this update webpack commit for issue #1032.

@GGAlanSmithee

This comment has been minimized.

Show comment
Hide comment
@GGAlanSmithee

GGAlanSmithee Nov 8, 2016

Contributor

I have had similar problems with webpack 2, and others have too. Not sure if it's related, but could be.

See webpack/webpack#959 search the comments for DedupePlugin, webpack/webpack#959 (comment) for example.

Contributor

GGAlanSmithee commented Nov 8, 2016

I have had similar problems with webpack 2, and others have too. Not sure if it's related, but could be.

See webpack/webpack#959 search the comments for DedupePlugin, webpack/webpack#959 (comment) for example.

@samit4me

This comment has been minimized.

Show comment
Hide comment
@samit4me

samit4me Nov 8, 2016

Contributor

Thanks for the link! I cannot believe how many people are having issues. After trawling through the list I tried the following individually and in combination with each other but no deal 🙅‍♂️:

  • chunksSortMode: 'none',
  • chunksSortMode: 'dependency',
  • minChunks: Infinity, // seems like a pointless exercise but didn't work
  • changing name: 'vendor' to names: ['vendor'],
  • removing the CommonsChunkPlugin

The only thing that appears to work is removing the new webpack.optimize.DedupePlugin(),.

Did you ever manage to get it working in the projects you were having trouble with @GGAlanSmithee ???

Contributor

samit4me commented Nov 8, 2016

Thanks for the link! I cannot believe how many people are having issues. After trawling through the list I tried the following individually and in combination with each other but no deal 🙅‍♂️:

  • chunksSortMode: 'none',
  • chunksSortMode: 'dependency',
  • minChunks: Infinity, // seems like a pointless exercise but didn't work
  • changing name: 'vendor' to names: ['vendor'],
  • removing the CommonsChunkPlugin

The only thing that appears to work is removing the new webpack.optimize.DedupePlugin(),.

Did you ever manage to get it working in the projects you were having trouble with @GGAlanSmithee ???

@GGAlanSmithee

This comment has been minimized.

Show comment
Hide comment
@GGAlanSmithee

GGAlanSmithee Nov 8, 2016

Contributor

Did you ever manage to get it working in the projects you were having trouble with @GGAlanSmithee ???

No, I gave up. My use-case was trying to extract the webpack manifest into its own chunk, to avoid re-hashing vendor chunks (to make long-term caching better). I decided it was a premature optimization and not worth the effort for the time being.

Contributor

GGAlanSmithee commented Nov 8, 2016

Did you ever manage to get it working in the projects you were having trouble with @GGAlanSmithee ???

No, I gave up. My use-case was trying to extract the webpack manifest into its own chunk, to avoid re-hashing vendor chunks (to make long-term caching better). I decided it was a premature optimization and not worth the effort for the time being.

@wer32

This comment has been minimized.

Show comment
Hide comment
@wer32

wer32 Nov 8, 2016

I also vote for remove this plugin. Currently this plugin make main......js less only for 10
kilos. For me it is not critical but of course better to have such kind of plugin in build. Are there exist another plugins with such kind of functionality?

wer32 commented Nov 8, 2016

I also vote for remove this plugin. Currently this plugin make main......js less only for 10
kilos. For me it is not critical but of course better to have such kind of plugin in build. Are there exist another plugins with such kind of functionality?

@Telemakhos

This comment has been minimized.

Show comment
Hide comment
@Telemakhos

Telemakhos Nov 10, 2016

So... I guess we can safely remove the line new webpack.optimize.DedupePlugin()in order to use Material-UI... Correct?

Telemakhos commented Nov 10, 2016

So... I guess we can safely remove the line new webpack.optimize.DedupePlugin()in order to use Material-UI... Correct?

@GGAlanSmithee

This comment has been minimized.

Show comment
Hide comment
@GGAlanSmithee

GGAlanSmithee Nov 10, 2016

Contributor

Here is an interesting development webpack/webpack#3266, so yes I think it's time to remove DedupePlugin ;)

not sure if this will fix anything though, doesn't look like it judging by @samit4me's experiments

Contributor

GGAlanSmithee commented Nov 10, 2016

Here is an interesting development webpack/webpack#3266, so yes I think it's time to remove DedupePlugin ;)

not sure if this will fix anything though, doesn't look like it judging by @samit4me's experiments

@samit4me

This comment has been minimized.

Show comment
Hide comment
@samit4me

samit4me Nov 10, 2016

Contributor

This is great, sokra himself is wanting to remove it, it must have issues. Thanks for the link @GGAlanSmithee. Removing the DedupePlugin certainly resolve the issue, its the only thing that has worked 😄.

I've retesting this morning and it works for both npm start and npm run start:production. Below is listed the steps I took:

Step One

Remove these two lines from webpack.prop.babel.js:

// Merge all duplicate modules
new webpack.optimize.DedupePlugin(),

Step Two

Replace the render function, with the following:

import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider';
import getMuiTheme from 'material-ui/styles/getMuiTheme';
import injectTapEventPlugin from 'react-tap-event-plugin';
injectTapEventPlugin();

const render = (messages) => {
  ReactDOM.render(
    <MuiThemeProvider muiTheme={getMuiTheme()}>
      <Provider store={store}>
        <LanguageProvider messages={messages}>
          <Router
            history={history}
            routes={rootRoute}
            render={
              // Scroll to top when going to a new page, imitating default browser
              // behaviour
              applyRouterMiddleware(useScroll())
            }
          />
        </LanguageProvider>
      </Provider>
    </MuiThemeProvider>,
    document.getElementById('app')
  );
};

Step Three

In FeaturePage/index.js

  • import RaisedButton from 'material-ui/RaisedButton';
  • <RaisedButton label="Default" />

Now you can run either npm start OR npm run start:production and it works in both 🎉. Note tho if you want to add material-up components to HomePage you will need to update the tests so that the components are wrapped in <MuiThemeProvider />, see #4021.

It certainly looks as tho we should remove it.

Contributor

samit4me commented Nov 10, 2016

This is great, sokra himself is wanting to remove it, it must have issues. Thanks for the link @GGAlanSmithee. Removing the DedupePlugin certainly resolve the issue, its the only thing that has worked 😄.

I've retesting this morning and it works for both npm start and npm run start:production. Below is listed the steps I took:

Step One

Remove these two lines from webpack.prop.babel.js:

// Merge all duplicate modules
new webpack.optimize.DedupePlugin(),

Step Two

Replace the render function, with the following:

import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider';
import getMuiTheme from 'material-ui/styles/getMuiTheme';
import injectTapEventPlugin from 'react-tap-event-plugin';
injectTapEventPlugin();

const render = (messages) => {
  ReactDOM.render(
    <MuiThemeProvider muiTheme={getMuiTheme()}>
      <Provider store={store}>
        <LanguageProvider messages={messages}>
          <Router
            history={history}
            routes={rootRoute}
            render={
              // Scroll to top when going to a new page, imitating default browser
              // behaviour
              applyRouterMiddleware(useScroll())
            }
          />
        </LanguageProvider>
      </Provider>
    </MuiThemeProvider>,
    document.getElementById('app')
  );
};

Step Three

In FeaturePage/index.js

  • import RaisedButton from 'material-ui/RaisedButton';
  • <RaisedButton label="Default" />

Now you can run either npm start OR npm run start:production and it works in both 🎉. Note tho if you want to add material-up components to HomePage you will need to update the tests so that the components are wrapped in <MuiThemeProvider />, see #4021.

It certainly looks as tho we should remove it.

@futzco

This comment has been minimized.

Show comment
Hide comment
@futzco

futzco Nov 11, 2016

@samit4me My apologies.

On the project I tested the issue, I had this custom script:

"build:dev": "cross-env NODE_ENV=development webpack --config internals/webpack/webpack.prod.babel.js --color -p"

and I run it out of habit instead of the standard build script that comes with react-boilerplate.

I am sincerely sorry for creating confusion due to my clumsiness.

futzco commented Nov 11, 2016

@samit4me My apologies.

On the project I tested the issue, I had this custom script:

"build:dev": "cross-env NODE_ENV=development webpack --config internals/webpack/webpack.prod.babel.js --color -p"

and I run it out of habit instead of the standard build script that comes with react-boilerplate.

I am sincerely sorry for creating confusion due to my clumsiness.

@Telemakhos

This comment has been minimized.

Show comment
Hide comment
@Telemakhos

Telemakhos Nov 11, 2016

Step Four:

Either remove sanitize.css or override defaults in global-styles.js, or some material-ui components like DropDown Menu will appear ugly as foark...

Before:
screen shot 2016-11-11 at 14 59 03

After:

injectGlobal`

  button, html [type="button"],[type="reset"], [type="submit"] {
    -webkit-appearance: none;
  }

`;

screen shot 2016-11-11 at 15 03 28

😝

Telemakhos commented Nov 11, 2016

Step Four:

Either remove sanitize.css or override defaults in global-styles.js, or some material-ui components like DropDown Menu will appear ugly as foark...

Before:
screen shot 2016-11-11 at 14 59 03

After:

injectGlobal`

  button, html [type="button"],[type="reset"], [type="submit"] {
    -webkit-appearance: none;
  }

`;

screen shot 2016-11-11 at 15 03 28

😝

@samit4me samit4me added bug and removed question labels Nov 12, 2016

@mattcrum

This comment has been minimized.

Show comment
Hide comment
@mattcrum

mattcrum Dec 7, 2016

@Telemakhos tossing that in global-styles.js didnt help me unfortunately. removing sanitize.css works but i'd prefer not to. Bummer, but it looks like it's just another issue with material-ui.

EDIT: works great actually. Some reason global-styles wasn't in my app.js file. :)

mattcrum commented Dec 7, 2016

@Telemakhos tossing that in global-styles.js didnt help me unfortunately. removing sanitize.css works but i'd prefer not to. Bummer, but it looks like it's just another issue with material-ui.

EDIT: works great actually. Some reason global-styles wasn't in my app.js file. :)

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 29, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock bot commented May 29, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2018

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