Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Update vendor js build #18

Merged
merged 9 commits into from
Sep 21, 2016
Merged

Update vendor js build #18

merged 9 commits into from
Sep 21, 2016

Conversation

macdonaldr93
Copy link
Contributor

@Shopify/themes-fed

Updating this build so that we can control the order that vendors get included. Also, added uglify. Doesn't seem to affect the length of time build takes that heavily, also, it would only run on watch if vendor.js was updated so I think it's fine.

Uglify will remove all comments except for license comments. We can leverage this to keep comments in the file if we want by doing: /*! lodash.core.min.js */.

I would like to bring this over to Debut.

@macdonaldr93 macdonaldr93 self-assigned this Sep 14, 2016
@cshold
Copy link
Contributor

cshold commented Sep 14, 2016

Add a comment into vendor.js that says the /* filename */ syntax is for keeping comments, otherwise I have no idea what's going on.

From a dev perspective, not running it on watch means if you ever add a vendor you have to restart your tasks. I'd rather have the uglification and upload take longer than it not happen at all. Thoughts?

@macdonaldr93
Copy link
Contributor Author

Added a comment that explains how the comment preservation works.

And yeah, like simply adding a file to the vendor folder won't trigger a rebuild; only updating vendor.js will. So I would agree with you, I'd rather it always happen and just take longer.

@cshold
Copy link
Contributor

cshold commented Sep 21, 2016

👍

@macdonaldr93 macdonaldr93 merged commit ac49379 into master Sep 21, 2016
@macdonaldr93 macdonaldr93 deleted the update-vendor-js-build branch September 21, 2016 19:01
@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants