-
Notifications
You must be signed in to change notification settings - Fork 3
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
If specific packages are found in project installation, gets versions from yarn and nodejs from these packages config #15
base: master
Are you sure you want to change the base?
Conversation
… from yarn and nodejs from these packages config
It seems tests failed due symfony/polyfill#174 issue. The fix was released but I think it can take some time to be available on CI environment. |
src/NodeComposerPlugin.php
Outdated
$this->updateConfigFromPackageProvidesConfig( | ||
$packageConfig, | ||
'node-version', | ||
'imponeer/composer-nodejs-installer', |
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 this a hard coded package name? What will happen if you decide to delete this package?
I see a lot of problems coming from 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.
Yeah, it's hardcoded package name. If you remove it, node-composer will work as before this package was installed. It means that you need specify node version manually in your package config. So, probably if you use this package and specify also minimal version in config, you will be insurred what will happen if imponeer/composer-nodejs-installer for some unlikly reason will be deleted as composer package. In that case user will loose auto updates, but he still could use own package from last hardcoded nodejs version.
I don't see much problems here but yeah I agree hardcoding package names is not a very good idea overall.
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 added posibility to specify any other package from where to get version with package-for-node-version
and package-for-yarn-version
config values.
For example, such composer json:
{
"name": "dummy/dummy",
"description": "Dummy project to test if configuration works",
"type": "project",
"require": {
"mariusbuescher/node-composer": "@dev",
"something/yarn": "1.0.0",
},
"extra": {
"mariusbuescher": {
"node-composer": {
"package-for-yarn-version": "something/yarn",
"node-version": "6.11.0"
}
}
}
}
Will install yarn 1.0.0 and nodejs 6.11.0
Or if such composer.json:
{
"name": "dummy/dummy",
"description": "Dummy project to test if configuration works",
"type": "project",
"require": {
"mariusbuescher/node-composer": "@dev",
},
"extra": {
"mariusbuescher": {
"node-composer": {
"package-for-yarn-version": "something/yarn",
"node-version": "6.11.0"
}
}
}
}
Will install only node 6.11.0 because something/yarn
is not required in project, so no yarn version could be resolved.
Right now is not fully dynamically (tries to find if imponeer packages are installed if not other specified) but I think this will solve most issues with what if these imponeer packages will disapeer someday
but if they are installed your package will have just some new extra functionality that will not break old packages but if user decides to use it will add some extra value and some possibilities.
The idea is definitely better than choosing the best version, but still problematic. I think it would be better to keep the plugin as is and add an update hook, that logs a message on update. Something like 'Hey, there is a new version of node.js availible. Would you like to update?' IMHO this will cause less trouble. Also the original intention of this package was to pin the node.js version of a project to one specific version, as I had multiple different projects running on my machine with incompatible versions of node.js. So I think these features doing magic with versions are quite dangerous and misleading. I also think they do not match with the intention of this plugin. |
Why I'm trying to add such functionality? Right now there is now way to easy to deal with nodejs security updates. Such hook will not help much, because project owner needs to run Why I need such functionality? I want to develop assets library that needs nodejs and npm, for compiling laravel mix and/or symfony encore assets for pluginable CMS'es. And for that I need some way to know when dependencies (and nodejs) was updated, because dealing with many libraries dependencies updates without any tools like dependabot is really a pain is the ass :| I really would like to use your library for my project than trying to create yet another one. Maybe is there a way to come up with something would be good enough for both of us? |
I totally support the idea of dealing with security update automatically, but it should not be built into a library itself, except it is the sole purpose of this library. Minimalism is something I try to embrace in libraries, so the complexity stays low and is handleable. Adding a features also adds the need to support this. When the feature is planned for one purpose, but not designed to only be used in this purpose, you as a maintainer have the duty to support all use-cases you never even thought about. Reviewing your pull request lets me see a lot of opportunities for an unplanned use. I'll give you an example. There is a library, that also ships with a JavaScript part to handle frontend stuff. In the downloadable artefact, the JavaScript library is already bundled in its built and compressed form. To run the build, this composer plugin is used in the library. You include this library in your project which has a build-process for the frontend using an ancient version of node.js. To make it manageable you also use this composer plugin, but the library ships with a newer version of node. Your pinned, ancient node version is just overridden by the library. In that case, it is totally unintended. That will lead to problems, because it is not very intuitive though. |
Your original idea of a github workflow was quite good though. Maybe you can add it in another repo and mention it in the |
Similar to #14 but instead of having multiple version conflicts, correct nodejs and yarn versions are detected from imponeer/composer-nodejs-installer and imponeer/composer-yarn-installer package data. So, if any of these packages are installed in project, config values will be overwritten from these packages data.
F.e., if project has such composer.json:
Node 1.0.0 will be installed
or if project has such composer.json:
Node 1.0.0 and yarn 1.0.0 will be installed
or if project has such composer.json:
Latest nodejs and yarn 1.0.0 version will be installed
or if such composer.json:
Yarn 1.0.0 and node 6.11.0 will be installed
However if both ways are defined, data from packages (not extras will take a priority). So, that's means that such composer.json config:
This would install yarn 1.0.0 and node 6.11.0
Of cource it will be possible to use composer version constraints for such new functionality. So, it would be easier to update package nodejs versions.
Yeah, I know this is a bit hardcoded that is not very good practice but I think this would be maybe a bit better way to solve this problem than #14 and this doesn't required to solve a big problem how to handle multiple installed nodejs and yarn versions. What do you think?