Feature/brunch1 #250

ajthomascouk · 2017-10-09T15:35:38Z

No description provided.

neilbmclaughlin · 2017-10-16T14:46:21Z

We discussed this in the dev catchup and the feeling is that brunch should be a dev dependency since it would be preferable if it wasn't deployed to production. The reason being that it is an unnecessary surface area in terms of security risk. This is pertinent given the snyk warning. Do you think this can be resolved given that you run brunch specifically when the env is production?

🐛 add call to correct script 🐍 regenerate yarn.lock following rebase

st3v3nhunt

I've got a few points/questions recorded on this PR. They are as much for myself to remind me what I need to change as anything.
I've got a branch with additional changes in that I'll finish and submit a PR into this one once I've done it.
Which is a long way to say, don't bother making these changes as I'll be doing it.

Should the assets referenced within the css be hashed too? What are the implications of the externally hosted assets e.g. the fonts using this approach?

st3v3nhunt · 2017-10-18T12:52:26Z

Dockerfile

@@ -1,5 +1,8 @@
 FROM node:8.4.0-alpine

+RUN apk add --no-cache python=2.7.13-r1 git-perl bash make gcc g++


Why are all these packages needed?

@markysoft can you explain please?

Brunch relies on node-sass, which requires python.
node-sass bundles a number of pre-compiled binaries for popular distributions, but not the flavour of linux we use.

I've added lines to the Docker file to install the pre-requisites as decribed in the issue sass/node-sass#1589

The issue has been fixed in master on node-sass, but it has not been released since May

st3v3nhunt · 2017-10-18T15:46:58Z

Dockerfile

@@ -23,6 +26,6 @@ USER root
 RUN find /code -user 0 -print0 | xargs -0 chown "$USERNAME":"$USERNAME"
 USER $USERNAME

-RUN [ "yarn", "run", "build-css" ]
+RUN yarn run brunch-build


There is no need to have run here

Hmmm, thought i tried it without. Okay.

st3v3nhunt · 2017-10-18T15:47:19Z

brunch-config.js

+      sourceMaps: false,
+      plugins: {
+        afterBrunch: [
+          'sleep 1s && yarn map-replace app/views/layout.nunjucks < assets.json'


Where does this come from?

st3v3nhunt · 2017-10-18T15:47:37Z

brunch-config.js

+  },
+  overrides: {
+    development: {
+      sourceMaps: false,


Why are source maps not used in development?

I don't use them. But I guess "this is for everyone" So i'll put them back.

Don't bother making any changes. See #250 (review)

st3v3nhunt · 2017-10-18T15:51:08Z

brunch-config.js

+    fingerprint: {
+      srcBasePath: 'public/',
+      destBasePath: 'public/',
+      hashLength: 8


No need for this as 8 is the default value

st3v3nhunt · 2017-10-18T15:51:28Z

package.json

@@ -4,7 +4,7 @@
  "description": "Web app to display useful information about GP practices Pharmacies",
  "main": "server.js",
  "scripts": {
-    "build-css": "./css-build",
+    "brunch-build": "yarn brunch build --production",


--production isn't needed.

st3v3nhunt · 2017-10-18T15:51:40Z

package.json

@@ -18,7 +18,7 @@
    "precommit": "yarn git-hook",
    "prepush": "yarn git-hook && yarn snyk test",
    "start": "node app.js",
-    "start-watch": "nodemon app.js | ./node_modules/bunyan/bin/bunyan & nodemon -e scss -x yarn build-css -- compact",
+    "start-watch": "yarn brunch watch & nodemon app.js | ./node_modules/bunyan/bin/bunyan",


This should be running as development

* Use source maps in dev * Clear old hashed files (for dev) * Remove default and otherwise unnecessary values

ajthomascouk · 2017-10-19T07:47:26Z

@st3v3nhunt wrote:

Should the assets referenced within the css be hashed too? What are the implications of the externally hosted assets e.g. the fonts using this approach?

Are the images cached? They change so rarely, do we need to bother?

st3v3nhunt · 2017-10-19T11:10:52Z

PR #256 created for some tweaks to this PR.
The variable qs (in https://github.com/nhsuk/profiles/pull/250/files#diff-624f0d73fb727e28599e9c0cafef6db5L17) is used for cache busting the JS too.
As part of this change it would be good to get the JS done in the same way.

Feature/brunch tweaks

st3v3nhunt · 2017-10-30T12:13:58Z

Superseded by #250

neilbmclaughlin added the WIP label Oct 11, 2017

ajthomascouk force-pushed the feature/brunch1 branch from 1d0c0c1 to 30162a7 Compare October 17, 2017 08:32

ajthomascouk mentioned this pull request Oct 17, 2017

🐷 add brunch #249

Closed

ajthomascouk requested a review from neilbmclaughlin October 17, 2017 10:18

ajthomascouk removed the WIP label Oct 17, 2017

ajthomascouk added 6 commits October 17, 2017 14:02

🐷 add brunch

02bcc2b

🐛 add call to correct script 🐍 regenerate yarn.lock following rebase

🐊 snyk vulerability update

3322426

🐷 add brunch

4c1a1e5

🐛 add call to correct script 🐍 regenerate yarn.lock following rebase

🐊 snyk vulerability update

6be3b42

👪 move brunch and dependants to dev-dependencies

caa038a

👽 fix error in package.json

37a1edc

markysoft force-pushed the feature/brunch1 branch from 9094506 to 37a1edc Compare October 17, 2017 13:17

ajthomascouk requested review from st3v3nhunt and removed request for neilbmclaughlin October 17, 2017 14:52

st3v3nhunt suggested changes Oct 18, 2017

View reviewed changes

st3v3nhunt added 3 commits October 18, 2017 17:03

🙈 Do not checkin css maps

0e49f60

📦 Update dependency policy along with dependencies

63c2e7b

🌀 Configure Brunch in dev and prod environments

b4f59b0

* Use source maps in dev * Clear old hashed files (for dev) * Remove default and otherwise unnecessary values

🚮 No need to have empty file

23f1976

Merge pull request #256 from nhsuk/feature/brunch-tweaks

c491507

Feature/brunch tweaks

st3v3nhunt closed this Oct 30, 2017

st3v3nhunt deleted the feature/brunch1 branch October 30, 2017 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/brunch1 #250

Feature/brunch1 #250

ajthomascouk commented Oct 9, 2017

neilbmclaughlin commented Oct 16, 2017 •

edited

Loading

st3v3nhunt left a comment •

edited

Loading

st3v3nhunt Oct 18, 2017

ajthomascouk Oct 19, 2017

markysoft Oct 19, 2017

st3v3nhunt Oct 18, 2017

ajthomascouk Oct 19, 2017

st3v3nhunt Oct 18, 2017

st3v3nhunt Oct 18, 2017

ajthomascouk Oct 19, 2017

st3v3nhunt Oct 19, 2017

st3v3nhunt Oct 18, 2017

st3v3nhunt Oct 18, 2017

st3v3nhunt Oct 18, 2017

ajthomascouk commented Oct 19, 2017 •

edited

Loading

st3v3nhunt commented Oct 19, 2017

st3v3nhunt commented Oct 30, 2017

		@@ -1,5 +1,8 @@
		FROM node:8.4.0-alpine

		RUN apk add --no-cache python=2.7.13-r1 git-perl bash make gcc g++

Feature/brunch1 #250

Feature/brunch1 #250

Conversation

ajthomascouk commented Oct 9, 2017

neilbmclaughlin commented Oct 16, 2017 • edited Loading

st3v3nhunt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajthomascouk commented Oct 19, 2017 • edited Loading

st3v3nhunt commented Oct 19, 2017

st3v3nhunt commented Oct 30, 2017

neilbmclaughlin commented Oct 16, 2017 •

edited

Loading

st3v3nhunt left a comment •

edited

Loading

ajthomascouk commented Oct 19, 2017 •

edited

Loading