-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
chore: package json cleanup #5649
Conversation
"build:lang": "npm-run-all build:lang:*", | ||
"build:lang:js": "vjslang --dir dist/lang", | ||
"build:lang:copy": "shx cp -R lang/* dist/lang/", | ||
"minify": "npm-run-all minify:*", | ||
"minify:js": "babel-node build/minify.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.
After the rollup tests pr is merged, I will work on a pr to use the shared rollup config, then the minify
script will go away entirely.
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.
Having rollup minify significantly increased build times because it had to be a separate rollup 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.
now that terser supports include/exclude
this won't be an issue.
83025fc
to
b9ad98e
Compare
build/assets.js
Outdated
.then(mapFiles) | ||
.then(function(files) { | ||
logTable(files); | ||
const filepaths = sh.find(path.join(__dirname, '..', 'dist', '**', '*.{js,css}')).filter(function(filepath) { |
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 using klawSync is a lot more readable than using sh.find
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.
So for me klawSync
wasn't actually working here. I did not see an ignore
option anymore. Now it seems you have to filter on your own. So klawSync
would be mostly the same. The path.join
is just so we can run this script from any directory and not just the root of the project.
}); | ||
} | ||
|
||
function logTable(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.
keeping these as named functions rather than inlining them into the promise chain increases readability of this file and the promise chain.
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.
For me it seemed like this file was very hard to follow with named functions and lot of the function arguments had weird argument names relative to what they actually were. I went back and looked at the file and found a few places to simplify it further and added comments.
|
||
const files = klawSync('sandbox/').filter((file) => path.extname(file.path) === '.example'); | ||
const files = sh.find(path.join(__dirname, '..', 'sandbox', '**', '*.*')) |
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.
klawSync is more readable, I think
build/sandbox.js
Outdated
.filter(function(change) { | ||
return !fs.existsSync(change.copy); | ||
}); | ||
.filter(function(change) { |
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.
does the linter fix this?
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 that it did
@@ -42,11 +41,20 @@ export default function generateExample({skipBuild} = {}) { | |||
sh.cp(path.join(vjsSwf, 'dist', 'video-js.swf'), swfDest); | |||
} | |||
|
|||
const files = klawSync('sandbox/').filter((file) => path.extname(file.path) === '.example'); | |||
const filepaths = sh.find(path.join(__dirname, '..', 'sandbox', '**', '*.*')) |
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.
klawSync is more readable, I think
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 could use just sandbox/
for find
too, but then the scripts don't work if run from somewhere other than the root dir
General comment about |
f435e2a
to
1d7e7ce
Compare
6f01bfd
to
89fae82
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
ed52a8f
to
7675c35
Compare
7675c35
to
33ada18
Compare
33ada18
to
7350c96
Compare
7350c96
to
06c552c
Compare
06c552c
to
de93caf
Compare
a260e20
to
b21d893
Compare
package.json
Outdated
"remark --output --", | ||
"git add" | ||
], | ||
"lang/*.json": [ |
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.
automatic translation updates on commit when they are changed!
b21d893
to
88d7c9c
Compare
88d7c9c
to
967360d
Compare
@@ -1,19 +0,0 @@ | |||
var safeParse = require("safe-json-parse/tuple"); |
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.
This file was removed in favor of doing what we do in the generator with not-prelease
3b0b4dc
to
d9e7a6d
Compare
d9e7a6d
to
554bc3e
Compare
build/assets.js
Outdated
// find all js/css files in the dist dir | ||
// but ignore any files in lang, example, or font directories | ||
const filepaths = sh.find(path.join(__dirname, '..', 'dist', '**', '*.{js,css}')).filter(function(filepath) { | ||
if ((/\/(lang|example|font)\//).test(filepath)) { |
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.
you could just return !(/.../.test(filepath);
build/generate-example.js
Outdated
} | ||
}; | ||
|
||
generateExample({skipBuild: true}); |
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 think we need to run this on require.
package.json
Outdated
@@ -18,6 +18,7 @@ | |||
"author": "Steve Heffernan", | |||
"scripts": { | |||
"sandbox": "node build/sandbox.js", | |||
"assert": "node build/assets.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.
This already exists as assets
script.
package.json
Outdated
"prepublishOnly": "run-p build", | ||
"publish": "node build/gh-release.js", | ||
"version": "node build/version.js && git add CHANGELOG.md", | ||
"update-changelog": "conventional-changelog -p videojs -i CHANGELOG.md -s", |
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 already have the changelog
script.
bluebird
- use native promises insteadklawsync
- useshelljs
babel/*
- lots of unused/uneeded babel packages, especially after converting fromimport
torequire
for our build scriptsminimist
- Was only used in one place, and we can just manually look for the one argument.tsml
no longer maintainedlint-staged
to update thetranslations-needed
doclint-staged
to lint and fix docs