-
Notifications
You must be signed in to change notification settings - Fork 297
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
Manage frontend dependencies with npm/yarn and webpack #1072
Manage frontend dependencies with npm/yarn and webpack #1072
Conversation
I don't think I can build the |
docker/alpine/Dockerfile.master
Outdated
@@ -1,4 +1,4 @@ | |||
FROM alpine:3.6 | |||
FROM mhart/alpine-node:9 |
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.
at this point, it would be worth migrating to a Docker multi-stage build to avoid providing an image embedding development resources (Composer and NodeJS) that are not needed to run the application
I can work on this in a separate issue/PR, as there's a bit of Docker involved in the next release with #1010
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.
Oh! I've never heard of multi-stage build, it looks great! I'll try to play with it in another PR.
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've started to work on this to use the composer image to download the Shaarli archive and resolve PHP deps :)
webpack.config.js
Outdated
|
||
const extractCssVintage = new ExtractTextPlugin({ | ||
filename: "../css/[name].css", | ||
publicPath: 'tpl/default/css/', |
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.
s/default/vintage/
webpack.config.js
Outdated
{ | ||
entry: { | ||
shaarli: './assets/vintage/js/base.js', | ||
picwall: './assets/default/js/picwall.js', |
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.
s/default/vintage/ (not sure about this one... are resources from default
needed by vintage
?)
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.
They are. But maybe I should add a common
folder instead?
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, I think it would make things clearer and help isolating template-specific resources
assets/default/js/base.js
Outdated
import Awesomplete from 'awesomplete'; | ||
|
||
// CSS | ||
import '../scss/shaarli.scss'; |
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.
referring to SASS / assets from JS code should not be necessary; asset copy and generation should be handled by Webpack
Thanks for the early review. I only had experience with webpack on POCs and such. I'll probably try to redo everything step by step to get a clean commit log. The global diff will probably be huge anyway. Note that I'll also use real ES6 syntax here and run |
Yep, Webpack has an awful lot of dependencies. Do we actually need it for our use case? It seems very powerful to create full-fledged frontend application bundles (as in Angular/React/Vue + Node + SASS/SCSS), but quite overkill to package a handful of dependencies. For similar use cases (backend serving static pages with 3rd-party CSS/JS), I went back to using:
Ping @nicolasdanelon and @tcitworld who contributed to the discussion in #755 :) |
Relates to shaarli#755 Relates to shaarli#1072 See: - https://docs.docker.com/develop/develop-images/multistage-build/ - https://hub.docker.com/r/library/composer/ - https://github.com/composer/docker - https://github.com/docker-library/docs/tree/master/composer Signed-off-by: VirtualTam <[email protected]>
These are devDependencies, not the project dependencies. |
Our needs here are quite basic:
Dependencies (and overhead) are worth keeping to a minimum, so we can easily maintain our build toolchain while avoiding headaches 🤕 |
Well, who can do more can do less. I'm sure we can do everything using other tools packaged in a single Makefile target, but once configured webpack does everything easily and has a lot of resources since it has more or less became a standard. Regarding our build toolchain and dev tools, I would tend to say that the ease of use overcomes their weight. |
29a5b47
to
d05d751
Compare
d05d751
to
003118c
Compare
Ok I think I'm mostly done here. I've entirely rewritten the commit log to make everything more understandable, even though the JS rewrite diff isn't really readable. Also I was pleasantly surprise to see that yarn is already installed by default in Travis PHP image. |
7dcf0b5
to
5c7caf4
Compare
"rules": { | ||
"no-param-reassign": 0, // manipulate DOM style properties | ||
"no-restricted-globals": 0, // currently Shaarli uses alert/confirm, could be be improved later | ||
"no-alert": 0, // currently Shaarli uses alert/confirm, could be be improved later |
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.
time to raise some issues then :)
I'm about to do the same for the TODOs concerning usage of die()
and exit()
in PHP code
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.
Issue raised in #1095
before_script: | ||
- PATH=${PATH//:\.\/node_modules\/\.bin/} | ||
script: | ||
- make clean | ||
- make check_permissions | ||
- make eslint |
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.
frontend lint & tests should be run in a dedicated JS / Node matrix to avoid cluttering the PHP test environments
this will be a bit tricky so it's OK to address this point in a separate PR
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.
Tested on my local development environment, works fine :)
@ArthurHoaro do you want to wait until a PR addressing Docker builds is submitted before merging?
I think that we should wait, otherwise the |
"font-awesome": "^4.7.0", | ||
"pure-extras": "^1.0.0", | ||
"purecss": "^1.0.0" | ||
}, |
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.
Should we setup some automation to warn us about new upstream releases of the dependencies? (or can yarn
do that for us?)
Great patch! It adds requirements for yarn+node sources.lists + packages in the build environment, but I can live with that :) Looks definitely more manageable.
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.
@nodiscc Github provides such a feature:
- https://help.github.com/articles/listing-the-packages-that-a-repository-depends-on/
- https://help.github.com/articles/listing-the-projects-that-depend-on-a-repository/
- https://help.github.com/articles/viewing-and-updating-vulnerable-dependencies-in-your-repository/
The list of supported languages and tools is growing :)
5c7caf4
to
d7eb06b
Compare
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to shaarli#1072 Changed: - update paths to resource files (assets, images) Removed: - references to resources now resolved through NPM - licenses corresponding to the aforementioned resources Signed-off-by: VirtualTam <[email protected]>
Relates to #755
JS:
CSS:
Images:
compressed at compilation by image-webpack-loaderUsage with
npm
andyarn
installed:TODO in this PR:
vintage
themeDocker images(do it in another PR)TODO list for later:
JSLint: fix 1k JS style errors and add it to Travis build