Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use npm in webpack, not checked in bower packages #15818

Merged
merged 16 commits into from
Feb 17, 2017
Merged

Use npm in webpack, not checked in bower packages #15818

merged 16 commits into from
Feb 17, 2017

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Feb 13, 2017

What does this change?

  • directs webpack to npm, rather than bower

What is the value of this and can you measure success?

  • will mean require/bower can go as one, when the time is right. there's no need to use bower

Does this affect other platforms - Amp, Apps, etc?

no

@sndrs sndrs requested a review from SiAdcock February 13, 2017 15:12
@PRBuilds
Copy link

PR build results:

screenshots
mobile.pngwide.pngtablet.pngdesktop.png

exceptions (1)
thrown-exceptions.js

webpagetest results
performanceComparisonSummary.txt

-automated message

Copy link
Contributor

@SiAdcock SiAdcock left a comment

Choose a reason for hiding this comment

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

👍 Amazing

@sndrs
Copy link
Member Author

sndrs commented Feb 14, 2017

hit a blocker:

currently we serve videojs@5.4.6 and videojs-ima@0.3.0 (both checked in), but videojs-ima@0.3.0 specifies a dependency of videojs@~5.3. webpack therefore is bundling both, adding 200k to the bundle... trying to work out how to handle it

@sndrs
Copy link
Member Author

sndrs commented Feb 14, 2017

raised an issue googleads/videojs-ima#333

@sndrs
Copy link
Member Author

sndrs commented Feb 15, 2017

tempted to downgrade our version till ima updates - seems like it might be a while, and we're running (potentially) incompatible versions as it is.

we'll mainly miss out on videojs/video.js#2638.

any feelings either way @SiAdcock @akash1810?

@akash1810
Copy link
Member

tempted to downgrade our version till ima updates - seems like it might be a while

@sndrs 👍 but could we do this in a test to be sure there isn't a detrimental loss of video consumption as a result?

@sndrs
Copy link
Member Author

sndrs commented Feb 16, 2017

how long would we have to run it for? according to #11821, it was only to bring it inline with contribs anyway?

@akash1810
Copy link
Member

Ah, I wasn't aware of that! In that case, I'm all for downgrading.

@sndrs sndrs merged commit 92c4d67 into master Feb 17, 2017
@sndrs sndrs deleted the webpack-npm branch February 17, 2017 12:19
@prout-bot
Copy link
Collaborator

Overdue on PROD (merged by @sndrs 30 minutes and 4 seconds ago) What's gone wrong?

@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @sndrs 1 hour, 24 minutes and 47 seconds ago) Please check your changes!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants