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
Use Babel 7 & Webpack3 (via blocks) #1334
Use Babel 7 & Webpack3 (via blocks) #1334
Conversation
0b57e41
to
006d429
Compare
bef8397
to
bd1d386
Compare
The PR |
Why |
Webpack's configuration object is a low-level format, that nearly no-one grasps fully. Therefore it's error prone and not easily to relate single lines to the functionality it should enable. Therefore a composable higher-level abstraction was chosen, that makes obvious what shall be achieved, which can get expanded to the verbose lower-level syntax. |
I am worried that new contributors would need to learn an abstraction to make a new change, on the hand, since this is related on how the library is build and distributed I don't think it would change a lot. |
Indeed. But given the fact what it takes to learn configuring Webpack properly (also in its different versions) and how often there's a need to change this config later (very few) the readability was my main driver. |
Yep I agree here:
|
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.
two small comments
"url-search-params": "^0.6.1", | ||
"typescript": "^2.0.3" | ||
"webpack": "^3.10.0", | ||
"webpack-dev-server": "^1.16.5" |
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.
Do we need this here?
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.
It was just wrongly sorted in the package.json
beforehand. The dev dependency can be removed in the Karma branch, as until then the karma config has a dependency on the Webpack dev server
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.
Yes to make the tests run in this PR, will get removed later on
}, | ||
"browser": { | ||
"./lib/adapters/http.js": "./lib/adapters/xhr.js" | ||
}, | ||
"typings": "./index.d.ts", | ||
"dependencies": { | ||
"follow-redirects": "^1.2.5", | ||
"is-buffer": "^1.1.5" | ||
"is-buffer": "^1.1.5", |
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 don't see this used here, or was it there before ?
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.
It's just the comma, as pkginfo
was added 😂
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.
In the utils for now, later on we might get rid of it when moving to lodash
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.
LGTM
Umbrella: #1333