-
Notifications
You must be signed in to change notification settings - Fork 278
Version control javascript libraries with NPM #3124
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
Conversation
|
If we do this, I'd suggest going for npm (or yarn) fully for JS dependencies |
So also remove JS dependencies in I somehow never used I also think we should only do this in the next release so we have some time to discuss. |
I think it doesn't make sense to have 2 package managers for frontend dependencies, that's why I suggest using npm (or yarn, which can read the same package.json file) for all. |
d4096af to
3bfe280
Compare
|
The other packages are harder to either find (dataTables) or don't work 1-on-1 (font-awesome). I'll look at those in another PR. Reviewing is easiest commit by commit and by just skipping the monaco one as that's the one touching most files. |
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.
We need to update the docs for yarn as well and later update the Docker images.
|
sorry i missed it. it's missing for fedora though |
|
|
||
| domserver: composer-dump-autoload | ||
|
|
||
| dependencies: composer-dependencies node-dependencies |
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.
These should (all, also composer-dependencies-dev below) be added as phony dependencies under .PHONY to make it clear that no actual target with this name gets created.
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.
Hmmm, I see that composer-dependencies{,-dev} are actually recursive targets from the root Makefile, so that should then be renamed there too.
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 think I already renamed the composer-dependencies{,-dev} to dependencies{,-dev} in all files (so also the root one). I've now added the .PHONY targets in the webapp/Makefile. I think that's what you meant?
| $(addprefix maintainer-,conf install) clean-autoconf config distdocs \ | ||
| composer-dependencies composer-dependencies-dev \ | ||
| coverity-conf coverity-build | ||
| dependencies dependencies-dev coverity-conf coverity-build |
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.
Adding the phony targets here is not strictly necessary, as recursive targets are already marked phony in https://github.com/DOMjudge/domjudge/blob/main/Makefile.global#L94, but this doesn't hurt.
Either Composer managed those before, or those were checked into git. Now we track those with `yarn`: - Coloris - JQuery debounce - Monaco editor - Bootstrap-toggle - File-saver - d3 - nvd3 - Select2 D3 has a newer release but we can't upgrade because of the nvd3 which needs this specific Major version.
NPM is another packagemanager for JS. So we use composer first and when we can't find the package there we check with npm.
This change does require more changes for maintainer-install & any checkout.
If we do this it makes introducing MathJax a lot easier and we can now also version control other javascript libraries.