Webpack + Jest #12
Webpack + Jest #12
Conversation
fernandosouza
requested review from
brunobasto,
jbalsas,
robframpton and
mthadley
Fixs package.json
|
Hey @fernandosouza, thanks a lot for putting this together! I'll take a look tomorrow! |
|
Thx, @jbalsas. |
There was a problem hiding this comment.
Hey @fernandosouza, this looks quite good, I've just added some comments from a quick review.
Also, could we move to an .eslintrc similar to https://github.com/metal/metal-clay-components/blob/master/.eslintrc ? That would conform to our more current standards
| } | ||
| ); | ||
| this.fs.copyTpl( | ||
| this.templatePath('test/_Boilerplate.js'), this.destinationPath('test/' + this.componentName + '.js'), | ||
| this.templatePath('__tests__/_Boilerplate.js'), this.destinationPath('__tests__/' + this.componentName + '.js'), |
There was a problem hiding this comment.
Should this depend on the chosen testEnvironment?
There was a problem hiding this comment.
Not exactly. It's just a configuration option for Karma. I thought to keep the same folder name to avoid the condition of the test environment. This name is the default for Jest and for Karma we always have to specify the test folder. So, made more sense for me to use __tests__. Does it make sense?
There was a problem hiding this comment.
mmm... it makes sense for us as maintainers, but I doubt that if you pick Karma as a test environment you expect tests to be there...
| filename: './build/<%= buildFormat %>/<%= kebabCaseName %>.js'<% } %> | ||
| }, | ||
| plugins: [ | ||
| new webpack.optimize.ModuleConcatenationPlugin() |
There was a problem hiding this comment.
Based on metal/metal.js#251, maybe we'd like to add the commonsChunk plugin by default?
There was a problem hiding this comment.
Chunk means separate in another file what is common for components in a bundle. Right?
There was a problem hiding this comment.
Isn't the commonChunk plugin only useful for builds that have multiple entry points? I think webpack 3 already attempts to dedupe as much as possible (given that you don't even need to include the plugin anymore https://webpack.js.org/guides/migrating/#dedupeplugin-has-been-removed).
There was a problem hiding this comment.
Yeah, I was just pointing that if someone tries to create entry points based on this generator the duplicated IncrementalDOM dependency will trip her up as seen in the referenced metal.js issue. This is might be a big gotcha, so we can either set it as default, or simply document it somewhere...
There was a problem hiding this comment.
What if we add the resolve alias in the generator? The highest component will resolve dependencies correctly. With the documentation you've suggested, @jbalsas, developers who didn't use the generator will be able to a fix his setup.
|
@mthadley, @bryceosterhaus do you guys have any suggestions for the generator setup? Some things you'd like to see? Maybe we could add a jsx flavour as well (in a different issue 😁)... @Robert-Frampton, @brunobasto, thoughts on this? |
|
Hey @fernandosouza I tried running this with all the default options, and npm choked on the generated package.json file.
|
| import Component from 'metal-component';<% } %><% if (buildFormat === 'jquery') { %> | ||
| import JQueryAdapter from 'metal-jquery-adapter';<% } %> | ||
|
|
||
| import './<%= kebabCaseName %>.scss'; |
There was a problem hiding this comment.
I'm a little worried about importing the scss by default, since it constricts the bundling process quite a bit. What if someone else wanted to use this component but didn't want to webpack their styles? Just a thought, I'm open to discussion.
@jbalsas , what do you think?
There was a problem hiding this comment.
I'm not sure if there is a way to deal with scss with Webpack in another way. Webpack receives a Javascript entry file and handles everything from that. There is a plugin that we can use to extract the CSS part from the bundle to another file.
|
@Robert-Frampton. I sent a new commit that seems to fix the error. |
|
@jbalsas nothing in particular stands out to me. I haven't used this generator yet and usually don't use generators at all, so I don't have much input for this specific use case. Glad to see Jest and Webpack being added though, I think it will help first-timers. |
|
Hey @fernandosouza, I'm afraid we shouldn't change the package name. Is there any particular reason you did so? |
Hey, guys. Based on what Chema did for Clay Components, I've replaced Gulp to Webpack and Karma to Jest.
Please, give me feedbacks.