jcoreio / crater

Meteor/Webpack/React SSR app skeleton that runs your app code outside of Meteor Isobuild
ISC License
82 stars 10 forks source link

Fix json loading in node modules, prevent double loading meteor-config #49

Closed evelant closed 7 years ago

evelant commented 7 years ago

The current code breaks once you start to add more packages to node_modules because json does not get parsed. Updated the test to fix that. Added an exclude for meteor-config.json because meteor-imports-webpack-plugin adds a loader specifically for that file and it will crash due to being parsed twice if not excluded.

darkadept commented 7 years ago

Great! This fixes #47.

darkadept commented 7 years ago

This doesn't fix production mode though. You need to add the same fix to the prod webpack config.

evelant commented 7 years ago

@darkadept I updated the commit to include the prod config, thanks for pointing that out!

jedwards1211 commented 7 years ago

Thanks @fignuts! I'm wary about using /node_modules/ as a RegExp though, since it could include directories like my_weird_node_modules, even though I don't know why anyone would ever name a directory like that. Do you know what specifically it fixes?

darkadept commented 7 years ago

The same issue is present for CSS files that need to be included from node_modules.

This gives the same error: import '../../../node_modules/react-dates/lib/css/_datepicker.css';

You need to add /node_modules/ to the CSS include in webpack config as well.

@jedwards1211 @fignuts Is there a better regex that can be used instead of /node_modules/?

evelant commented 7 years ago

@jedwards1211 its likely that my understanding of webpack is lacking, but it seems that the string 'node_modules' did not match json files under the node_modules directory whereas the regex appears to match correctly.

The docs say A condition may be a RegExp (tested against absolute path), a string containing the absolute path, a function(absPath): bool, or an array of one of these combined with "and".

evelant commented 7 years ago

@jedwards1211 I just tested it and it needs to be the full path or a regex, just the string 'node_modules' does not work, but /node_modules/ or path.join(root, 'node_modules') works fine. In either case the exclude /meteor-config/ is still needed to get around the double compilation of the meteor config.

evelant commented 7 years ago

I updated the commit to use the specific path to node_modules to avoid any false positives from the regex.

jedwards1211 commented 7 years ago

@fignuts ohhh okay. Though I wonder if './node_modules' would work? In any case, I just made a PR to fix another issue with json files that just uses exclude instead of include, and it will probably fix this issue as well. What I did was exclude /meteor-imports-webpack-plugin/ which I think is a bit more specific than /meteor-config/. Actually I should just use an absolute path...

jedwards1211 commented 7 years ago

@fignuts okay, let me know if https://github.com/jedwards1211/crater/pull/50 works for you

evelant commented 7 years ago

@jedwards1211 that works great, thanks!

jedwards1211 commented 7 years ago

You're welcome!