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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby): compile themes through default webpack config #13651

Merged
merged 12 commits into from
May 14, 2019
Merged

fix(gatsby): compile themes through default webpack config #13651

merged 12 commits into from
May 14, 2019

Conversation

grantglidewell
Copy link
Contributor

Description

Apply default webpack config to gatsby themes @ChristopherBiscardi

Related Issues

Addresses:
ChristopherBiscardi/gatsby-theme-examples#10

@grantglidewell grantglidewell requested a review from a team as a code owner April 26, 2019 17:42
@grantglidewell
Copy link
Contributor Author

I dont think the test failure is related to the code changes:

make: Entering directory '/home/circleci/project/node_modules/sharp/build'
  TOUCH Release/obj.target/libvips-cpp.stamp
  CXX(target) Release/obj.target/sharp/src/common.o
../src/common.cc:654:1: error: expected ‘}’ at end of input
 }  // namespace sharp
 ^

ChristopherBiscardi
ChristopherBiscardi suggested changes May 5, 2019
View reviewed changes
packages/gatsby/src/utils/webpack.config.js Outdated Show resolved Hide resolved
packages/gatsby/src/utils/webpack.config.js Outdated Show resolved Hide resolved
ChristopherBiscardi and others added 2 commits May 5, 2019 06:05
@ChristopherBiscardi
Update packages/gatsby/src/utils/webpack.config.js
a32dec0 
Co-Authored-By: grantglidewell <this.prototype.grant@gmail.com>
@ChristopherBiscardi
Update packages/gatsby/src/utils/webpack.config.js
88806ad 
Co-Authored-By: grantglidewell <this.prototype.grant@gmail.com>
chrisdhanaraj
chrisdhanaraj reviewed May 10, 2019
View reviewed changes
Copy link

@chrisdhanaraj chrisdhanaraj left a comment

Choose a reason for hiding this comment

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

Hi! Sorry to jump in like this - pulled down this PR for my own use, thank you for opening it! I ran into a couple small issues when I was using this,

packages/gatsby/src/utils/webpack.config.js Outdated
if (
store.getState().themes.themes
) {
configRules.concat(
Copy link

Choose a reason for hiding this comment

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

this one probably needs to be

configRules = configRules.concat...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, true as concat would not alter the original array

packages/gatsby/src/utils/webpack.config.js Outdated
store.getState().themes.themes.map(theme => {
return {
test: /\.jsx?$/,
includes: theme.themeDir,
Copy link

Choose a reason for hiding this comment

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

Think the rule option is called include vs includes

https://webpack.js.org/configuration/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, good catch

@pieh pieh self-assigned this May 13, 2019
@wardpeet wardpeet changed the title added includes for themes in webpack config fix(gatsby): compile themes through default webpack config May 13, 2019
pieh
pieh approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM (also verified locally) 👍

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 14, 2019
@pieh pieh merged commit d5be306 into gatsbyjs:master May 14, 2019
@wardpeet
Copy link
Contributor 

Successfully published:
 - gatsby@2.4.6

@LekoArts LekoArts mentioned this pull request May 17, 2019
Merged
@jlengstorf jlengstorf mentioned this pull request May 21, 2019
Merged
@LekoArts LekoArts mentioned this pull request Jan 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdhanaraj chrisdhanaraj chrisdhanaraj left review comments

@pieh pieh pieh approved these changes

@ChristopherBiscardi ChristopherBiscardi ChristopherBiscardi left review comments

Assignees

@pieh pieh

Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grantglidewell @wardpeet @pieh @ChristopherBiscardi @chrisdhanaraj