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

Issue 14 add ci build and documentation for critical #16

Conversation

thescientist13
Copy link
Collaborator

@thescientist13 thescientist13 commented May 3, 2018

resolves #14

Overview

This PR aims to document this project's dependency on critical which was updated recently to use Headless Chrome (via puppeteer) (instead of PhantomJS).

This now requires consuming build time environments have support to run Headless Chrome.

Changes

  1. Added a CircleCi config file (using an image that supports Headless Chrome)
  2. Added consumer documentation around Headless Chrome / puppeteer
  3. Fixed some tests

Notes:

Before

Ran the build once using a "stock" Docker image, circleci/node:8.9.4 to help demonstrate the issue of the build needed Headless Chrome packages.

We can see that when the tests run, which invoke HtmlCriticalWebpackPlugin and ultimately critical, we get a node error attributed to puppeteer under the "Run Tests" section. (ideally this should cause the build to fail though but seems to be a mocha / node related issue)
screen shot 2018-05-09 at 5 19 51 pm

After

Then updated the PR to use a Docker image I put together specifically with the goal of being able to run Headless Chrome for NodeJS projects, and now we get the test results we're looking for! ✅
screen shot 2018-05-09 at 6 30 44 pm

My Docker image is pretty straightforward

FROM ubuntu:16.04

RUN apt-get update
RUN apt-get install -y curl vim git bzip2 ssh

# install HeadlessChrome X11 packages
# https://github.com/Quramy/puppeteer-example#with-docker-based-ci-services
RUN apt-get install -yq gconf-service libasound2 libatk1.0-0 libc6 libcairo2 libcups2 libdbus-1-3 \
      libexpat1 libfontconfig1 libgcc1 libgconf-2-4 libgdk-pixbuf2.0-0 libglib2.0-0 libgtk-3-0 libnspr4 \
      libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 \
      libxcursor1 libxdamage1 libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 \
      ca-certificates fonts-liberation libappindicator1 libnss3 lsb-release xdg-utils wget

# install NodeJS LTS
RUN curl -sL https://deb.nodesource.com/setup_8.x | bash -
RUN apt-get install -y nodejs

# install latest yarn@^1.0.0
# use yarn to install npm \_(ツ)_/¯
# https://stackoverflow.com/questions/44269086/how-to-upgrade-npm-to-npm5-on-the-latest-node-docker-image
RUN npm install -g yarn@^1.0.0

# upgrade to latest release of npm@^5.0.0
RUN yarn global add npm@^5.0.0

# environment info
RUN node -v
RUN npm -v
RUN yarn -v

TODO

  1. try and get CircleCI to fail in the "before" case where the build should fail because Headless Chrome wasn't started This seems out of scope for this PR, and should be tracked in the mocha issue tracker
  2. Update this ticket with results, screenshots, links to the builds, etc
  3. Should add developer documentation around using Docker

@thescientist13
Copy link
Collaborator Author

thescientist13 commented May 3, 2018

Hmm, this is still building against my fork and not from here
https://circleci.com/gh/anthonygore/html-critical-webpack-plugin

Might try and use this as an opportunity to see if there's a way to make sure CircleCI actually fails the build for a situation like this.

@thescientist13
Copy link
Collaborator Author

thescientist13 commented May 7, 2018

This support article suggested deleting any existing webhooks (which I had on my fork, and have now removed) and setting up the project again.

@anthonygore
Do you mind doing me a favor and deleting the project you made in CircleCI and setting up again? I think that should help just to be sure.

@anthonygore
Copy link
Owner

Done

@thescientist13
Copy link
Collaborator Author

thescientist13 commented May 7, 2018

thanks @anthonygore . still doesn't seem to be building.

could you please confirm a couple things?

  1. In GitHub, a new webhook was added to this project's GitHub setting's from CircleCI (should only be 1)
  2. In CircleCI, the build forked pull requests setting is set to "on" in the project's advanced settings
  3. In CircleCI, the free and open source setting is set to "on" in the project's advanced settings

thanks!

@anthonygore
Copy link
Owner

Hi Owen. All those settings are correct. I just created a branch and pull request to test it and triggered a build, so I'm not sure what the cause of your troubles are!

@thescientist13 thescientist13 force-pushed the issue-14-add-ci-build-and-documentation-for-critical branch from f88ef66 to 9d09dc0 Compare May 9, 2018 20:57
@thescientist13
Copy link
Collaborator Author

thescientist13 commented May 9, 2018

AH HA! There we go 😊 . I still had a project configuration for my fork in CircleCi, even though it wasn't building anything (I had only deleted the webhook in GitHub).

So I deleted it and 🎉 !

@anthonygore
Will update the description and let you know when everything is ready to review .

@thescientist13 thescientist13 force-pushed the issue-14-add-ci-build-and-documentation-for-critical branch 3 times, most recently from 5e08703 to 9d09dc0 Compare May 9, 2018 22:27
@thescientist13
Copy link
Collaborator Author

@anthonygore
All set for you to review! 👍

@anthonygore
Copy link
Owner

Brilliant! Thanks, Owen. I'll go through this shortly.

@anthonygore anthonygore merged commit 5a916be into anthonygore:master May 12, 2018
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.

critical 1.0.0 upgrade introduced (undocumented?) breaking change / dependency on puppeteer (Headless Chrome)
2 participants