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/copy zip clean migrate #5544

Merged
merged 5 commits into from Nov 8, 2018
Merged

Chore/copy zip clean migrate #5544

merged 5 commits into from Nov 8, 2018

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Nov 1, 2018

Migrates the clean, copy, and zip grunt tasks to npm scripts

After this pr there will probably be 1 more to convert the remainder to shell commands. From there there will be a pr to remove grunt entirely and to fixup the current shell commands in a way that makes sense.

@@ -17,6 +17,13 @@
"homepage": "https://videojs.com",
"author": "Steve Heffernan",
"scripts": {
"copy-fonts": "shx cp -R node_modules/videojs-font/fonts build/temp/font",
"copy-dist": "shx cp -R build/temp/* dist/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is necessary and if copy-fonts should just copy it directly into dist/font. Do we have a lot of other stuff getting built into build/temp?

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 a lot of this needs to be cleaned up, but for now I feel better just making it do exactly what it did

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, we can fix it up later.

@gkatsev
Copy link
Member

gkatsev commented Nov 7, 2018

Not sure what the changes are but I saw that the zip file created is only 2.5MB compared to currently where it is around 8.5MB.

Also, I think the package-lock is stale and needs to be regenerated.

@gkatsev
Copy link
Member

gkatsev commented Nov 7, 2018

Also, are there alternatives to cross-var? Maybe just a standard script? It seems to be depending on a babel at runtime, so, we have two babel versions being installed in our build tree.

@brandonocasey
Copy link
Contributor Author

if we don't care about dev support for windows we can drop it, we could also just use a script yeah

@brandonocasey
Copy link
Contributor Author

I saw the size change as well, but it appears the compression is just better. From a diff between the old zip and the new one it was the same.

@gkatsev
Copy link
Member

gkatsev commented Nov 8, 2018

Yeah, let's just leave it in for now but would be nice to either fix cross-var or do something else long term. Installing two babels isn't great.

Ah, interesting, didn't think about compression for the zip. unzip ls seems to confirm the same files exist.

@gkatsev
Copy link
Member

gkatsev commented Nov 8, 2018

Ok, confirmed that video.js inside the zip is equivalent to the file outside the zip. Must be different compression settings then.

@gkatsev gkatsev merged commit 2d682a4 into master Nov 8, 2018
@gkatsev gkatsev deleted the chore/copy-zip-clean-migrate branch November 8, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants