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

remove modules that is the relative path. #7934

Merged
merged 6 commits into from
Sep 13, 2018
Merged

remove modules that is the relative path. #7934

merged 6 commits into from
Sep 13, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Sep 6, 2018

What is this for? It breaks a lot stuff in weird subtle ways. e.g. react-popper requires a file popper.js from node_modules in Popper.js, this setting causes the import to resolve to itself instead of the other module of a similar name.

@jquense
Copy link
Contributor Author

jquense commented Sep 6, 2018

ah actually i don't think we want to set modules at all anymore, we aren't doing any fancy resolution, why are we messing with the default resolution algo?

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

some (not a lot) context for this - #7480 - I would be very happy to see this gone, but css modules stuff would probably need to be looked at again

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

maybe removing entire modules entry would actually fix that instead of this ugly hack I added ...

@jquense
Copy link
Contributor Author

jquense commented Sep 6, 2018

but css modules stuff would probably need to be looked at again

what is the check for that, i use css modules in all my gatsby projects, including ones where i remove this manually and haven't seen a problem. maybe i'm not hitting the edge case?

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

I've used this very basic reproduction https://github.com/timurc/gatsby-starter-default-css-module-image-test/ which was throwing can't find 'boa.png' ( https://github.com/timurc/gatsby-starter-default-css-module-image-test/blob/master/src/components/style.module.css - and image is sibling file to css module file)

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

what was also weird about this issue - that only combo of css modules + mini extract plugin was resulting in that - development (so style-loader instead of mini extract plugin) didn't have problem

@jquense
Copy link
Contributor Author

jquense commented Sep 7, 2018

BAH found the problem, is cssnano, running to early to minimize the styles and "cleaning" the url to a non relative one. fix incoming

@jquense
Copy link
Contributor Author

jquense commented Sep 7, 2018

OK fixed and minification is now done correctly per the webpack docs


const autoprefixer = require(`autoprefixer`)
const flexbugs = require(`postcss-flexbugs-fixes`)
const UglifyPlugin = require(`uglifyjs-webpack-plugin`)
const TerserPlugin = require(`terser-webpack-plugin`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a drop-in replacement but maintained unlike uglify-es

@@ -430,7 +415,7 @@ module.exports = async (
// https://github.com/defunctzombie/package-browser-field-spec); setting
// the target tells webpack which file to include, ie. browser vs main.
target: stage === `build-html` || stage === `develop-html` ? `node` : `web`,
profile: stage === `production`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think this was ever true? stage can't be production

Copy link
Contributor

Choose a reason for hiding this comment

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

haha yeah 🤔

@@ -112,6 +112,7 @@
"socket.io": "^2.0.3",
"string-similarity": "^1.2.0",
"style-loader": "^0.21.0",
"terser-webpack-plugin": "^1.0.2",
"type-of": "^2.0.1",
"uglifyjs-webpack-plugin": "^1.2.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed?

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looks good! One comment on the minimize option on webpack that's worth looking into.

packages/gatsby/src/utils/webpack-utils.js Show resolved Hide resolved
@@ -455,7 +440,11 @@ module.exports = async (
splitChunks: {
name: false,
},
minimize: !program.noUglify,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it lower. noUglify here would disable css minimization as well, which isn't what the argument suggests would happen

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Alrighty, looks good. Let's get those merge conflicts ironed out and we can merge later.

@pieh pieh merged commit 4be5cfd into master Sep 13, 2018
@pieh pieh deleted the jquense-patch-1 branch September 13, 2018 20:29
@jquense
Copy link
Contributor Author

jquense commented Sep 13, 2018

🎉 THANKS YA'LL

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.

4 participants