-
Notifications
You must be signed in to change notification settings - Fork 213
Support multiple entry points via new mains options, remove entry option #487
Conversation
https: false, | ||
contentBase: neutrino.options.source, | ||
open: false, | ||
hot: true, | ||
historyApiFallback: true, | ||
publicPath: '/', | ||
headers: { | ||
host: publicHost | ||
host: `${publicHost}:${port}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we include the port in the host
if it's defined in port
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't work everywhere without doing it like this. Or maybe it should only be set for public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just public
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it out and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I thought this was editing devServer.host
, but this is actually the HOST
header we are modifying. The host header needs the port as well, not just the domain/host portion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edmorley could you give this a sanity check?
Yeah definitely - I'll do so within the next 24 hours :-) |
Going to merge this, but feel free to review whenever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing, thank you!
The only other place that needed to be adjusted for multi-entrypoint that I could find was html-template, but that's been fixed since in:
https://github.com/mozilla-neutrino/neutrino-dev/pull/509/files#diff-f7eeb689dea27b680d5b7fd733b51ec9R167
plugins: ['react'] | ||
plugins: ['react'], | ||
rules: { | ||
'react/react-in-jsx-scope': 'off' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -53,10 +54,10 @@ module.exports = (middleware, args) => { | |||
compiler.plugin('done', () => { | |||
building.succeed('Build completed'); | |||
}); | |||
compiler.plugin('compile', () => { | |||
compiler.plugin('compile', debounce(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining why this is needed? (I thought Ora did debouncing? https://github.com/sindresorhus/ora#interval)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't debouncing the animation, rather this is debouncing the compile
event. If devServer
gets several updates in a row, like an editor writes multiple files at different times, the compiler will send several compile
events, which causes a lot of noise in the terminal. This tries to reduce that logging noise to just once per second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add comment. :)
@@ -53,4 +54,12 @@ module.exports = (neutrino, opts = {}) => { | |||
.set('react-dom', 'preact-compat') | |||
.set('create-react-class', 'preact-compat/lib/create-react-class') | |||
.set('react-addons-css-transition-group', 'preact-css-transition-group'); | |||
|
|||
neutrino.config.when(neutrino.config.module.rules.has('lint'), () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change related to the rest of the PR? I couldn't see anything explaining it in the commit message or review comments?
Also could you add a comment explaining why this is needed? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it so we can use JSX without having to manually say:
import React from 'react';
This was supposed to be part of the create-project fixes, but I did that work in tandem with this one because they were interlinked, and I tried to split them out, but unfortunately missed a few things.
Fixes #367. Still a WIP, and still need docs.