Skip to content
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

Merged
merged 3 commits into from
Aug 30, 2019
Merged

chore: package json cleanup #5649

merged 3 commits into from
Aug 30, 2019

Conversation

brandonocasey
Copy link
Contributor

  1. Remove packages that we really don't need:
  • bluebird - use native promises instead
  • klawsync - use shelljs
  • babel/* - lots of unused/uneeded babel packages, especially after converting from import to require for our build scripts
  • minimist - Was only used in one place, and we can just manually look for the one argument.
  • tsml no longer maintained
  1. Turns on linting for almost everything
  2. Uses lint-staged to update the translations-needed doc
  3. Uses lint-staged to lint and fix docs

"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",
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 3 times, most recently from 83025fc to b9ad98e Compare December 5, 2018 21:35
build/assets.js Outdated
.then(mapFiles)
.then(function(files) {
logTable(files);
const filepaths = sh.find(path.join(__dirname, '..', 'dist', '**', '*.{js,css}')).filter(function(filepath) {
Copy link
Member

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

Copy link
Contributor Author

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) {
Copy link
Member

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.

Copy link
Contributor Author

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', '**', '*.*'))
Copy link
Member

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) {
Copy link
Member

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?

Copy link
Contributor Author

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', '**', '*.*'))
Copy link
Member

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

Copy link
Contributor Author

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

@brandonocasey
Copy link
Contributor Author

General comment about klawSync I think it's doing more than we need in all but one script, ie find and stat when we just need find. I think removing the dependency is worth it in this case.

@gkatsev gkatsev added patch This PR can be added to a patch release. chore labels Dec 10, 2018
@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 2 times, most recently from f435e2a to 1d7e7ce Compare December 12, 2018 16:06
@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 2 times, most recently from 6f01bfd to 89fae82 Compare January 8, 2019 17:37
@stale
Copy link

stale bot commented Mar 9, 2019

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.

@stale stale bot added the outdated Things closed automatically by stalebot label Mar 9, 2019
@gkatsev gkatsev removed the outdated Things closed automatically by stalebot label Mar 12, 2019
@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 4 times, most recently from ed52a8f to 7675c35 Compare March 20, 2019 15:30
@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 2 times, most recently from a260e20 to b21d893 Compare July 17, 2019 17:31
package.json Outdated
"remark --output --",
"git add"
],
"lang/*.json": [
Copy link
Contributor Author

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!

@brandonocasey brandonocasey deleted the chore/package-json-cleanup branch July 18, 2019 19:12
@brandonocasey brandonocasey restored the chore/package-json-cleanup branch July 18, 2019 19:12
@brandonocasey brandonocasey reopened this Jul 18, 2019
@@ -1,19 +0,0 @@
var safeParse = require("safe-json-parse/tuple");
Copy link
Contributor Author

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

@brandonocasey brandonocasey force-pushed the chore/package-json-cleanup branch 2 times, most recently from 3b0b4dc to d9e7a6d Compare August 29, 2019 21:50
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)) {
Copy link
Member

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);

}
};

generateExample({skipBuild: true});
Copy link
Member

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",
Copy link
Member

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",
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants