Modernize manager template build workflow #15793
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,40 @@ | ||
| { | ||
| "name": "revolution-theme-default", | ||
| "title": "MODX Revolution", | ||
| "version": "3.0.0", | ||
| "license": "GPL-2.0+", | ||
| "repository": "https://github.com/modxcms/revolution", | ||
| "devDependencies": { | ||
| "@lodder/grunt-postcss": "^3.0.1", | ||
| "autoprefixer": "^10.3.3", | ||
| "bourbon": "^5.1.0", | ||
| "bourbon-neat": "^3.0.0", | ||
| "cssnano": "^5.0.8", | ||
| "grunt": "^1.4.1", | ||
| "grunt-contrib-concat": "^1.0.1", | ||
| "grunt-contrib-copy": "^1.0.0", | ||
| "grunt-contrib-watch": "^1.1.0", | ||
| "grunt-dart-sass": "^2.0.1", | ||
| "grunt-notify": "^0.4.5", | ||
| "grunt-terser": "^2.0.0", | ||
| "node": "^14.15.0", | ||
| "normalize-scss": "^7.0.1", | ||
| "postcss": "^8.3.6" | ||
|
There was a problem hiding this comment. Why you use postcss and sass both? There was a problem hiding this comment. postcss is being used as a base for cssnano and autoprefixer. I'm not using the postcss scss plugin to parse scss because it's not entirely standard sass. |
||
| }, | ||
| "dependencies": { | ||
| "@fortawesome/fontawesome-free": "^5.15.4" | ||
| }, | ||
| "scripts": { | ||
| "build": "grunt build" | ||
| }, | ||
| "browserslist": [ | ||
| "Chrome >= 64", | ||
| "Firefox >= 78", | ||
| "Safari >= 12", | ||
| "Edge >= 79", | ||
| "iOS >= 11", | ||
| "ChromeAndroid >= 92", | ||
| "last 2 Android versions", | ||
| "not dead" | ||
| ] | ||
smg6511 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
windows 7 is not supported by node.js 14 :( Windows 7 still be more popular than 8, at least in Russia.
Is this dependency is necessary?
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.
Windows 7, as you probably know, has been unsupported by Microsoft for more that a year and a half now. That said, I've seen that people on 7 have been able to get node 14+ running. See nodejs/node#33000 (comment)
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.
Thanks for link, I'll try it.
Anyway, I would ask again: for what purpose is this dependency?
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 the base software needed to use npm and the modules it manages. Are you intending on doing some development on the manager interface? Note that this same dependency has existed in 2.x for quite a while now.
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 very strange to have the npm installed and not have the nodejs installed, isn't it?
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.
@mishantrop yes it is. I assume you're running Windows 7?
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, including. Also I use Fedora and Mac OS. Dependings on tasks.
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.
Also there are better ways to limit the version of the application (anyway, I dont see any reasons for it in MODX):
package.json{ "name": "modx", "version": "3.0.0", "engines": { "node": ">=12.0.0" } }.npmrcI have not personally tested it as it was never necessary.
Source: https://stackoverflow.com/a/68653111