fix: remove gulp from dist folder build pipeline #1385

jbadan · 2019-04-10T21:10:44Z

This pr removes gulp from the dist folder creation pipeline. To test that the output was the same, I used an AST diffing tool. Results can be seen here: https://gist.github.com/jbadan/b3599f727927ddfc9e93d149d297d794

There is also a branch available that creates a new dist2 folder that can be used to compare and run the diffs yourself, npm run build:dist:prod, npm run build:dist2:prod and npm run diff:forEach on branch diff-compare-dist-dist2. This branch isn't quite as organized, but the scripts are doing the same thing.

Previous gulp tasks and their replacements are as follows:

pkg-clean: npm run style:remove
pkg-fonts, pkg-icons, pkg-images: npm run style:assets
pkg-css:
1. sassTask and componentTask: npm run style:compile
2. darkSassTask: never run, removed
3. minifyTask: npm run style:minify
4. bannerTask and the autoprefix and cleanCSS steps in the previous pkg-css tasks: npm run style:postCSS
5. various renames: npm run style:rename

Note: error-handling was only checking docs site so should not have been part of this workflow.

There are several followup tasks to completely remove gulp:

rework less folder build gulp task
rework docs site gulp tasks
rework backstop gulp tasks
rework "create" task for creating templates for a new component
update all wiki docs with references to gulp tasks

Once complete, we can remove the following from package.json:

    "gulp": "^4.0.0",
    "gulp-autoprefixer": "^6.0.0",
    "gulp-clean-css": "^3.10.0",
    "gulp-cli": "^2.0.1",
    "gulp-footer-secured": "^2.0.2",
    "gulp-header": "^2.0.5",
    "gulp-if": "^2.0.2",
    "gulp-imagemin": "^5.0.3",
    "gulp-nodemon": "^2.4.2",
    "gulp-notify": "^3.2.0",
    "gulp-rename": "^1.4.0",
    "gulp-replace": "^1.0.0",
    "gulp-sass": "^4.0.2",
    "gulp-sourcemaps": "^2.6.4",
    "require-dir": "^1.1.0",
    "signale": "^1.3.0",
    "undertaker-forward-reference": "^1.0.2",
    "wait-for-port": "0.0.2",

joseegm · 2019-04-10T21:14:12Z

Test Version for this PR was deployed

Built with commit b14b220

https://deploy-preview-1385--fundamental.netlify.com

jbadan · 2019-04-11T21:22:46Z

ops/gulp/pkg-css.js

@@ -51,71 +41,5 @@ const sassTask = () => {
 };
 gulp.task('pkg-sass', sassTask);


This task is still needed for the docs site build.

greg-a-smith · 2019-04-12T14:52:07Z

@jbadan Is there a story (or stories) created for the 5 follow-up tasks?

package.json

greg-a-smith · 2019-04-12T14:58:37Z

package.json

+    "style:minify": "foreach -t -g \"dist/*.css\" -x \"cleancss -O1 specialComments:1 #{path} -o #{dir}/#{name}.min.css\" --no-c && foreach -t -g \"dist/components/*.css\" -x \"cleancss -O1 specialComments:1 #{path} -o #{dir}/#{name}.min.css\" --no-c",
+    "style:postCSS": "postcss --config config/postcss.config.js  --replace dist/**/*.css && postcss --config config/postcss.config.js  --replace dist/*.css",
+    "style:remove": "if [ -d dist/ ]; then rm -rf dist/; fi",
+    "style:rename": "mv dist/all.css dist/fiori-fundamentals.css && mv dist/all-ie11.css dist/fiori-fundamentals-ie11.css && rm dist/fundamentals-ie11.less.css dist/fundamentals.less.css",


I see why all these were created, but would they ever be run on their own? For debugging, I suppose, but this clutters the scripts section quite a bit for items that theoretically are just steps of a larger script and probably shouldn't be run on their own. Because I'm assuming if you run them out of sequence, you'll get unexpected results and/or errors.

I'm not totally against this, but just making a note to see if anyone else has thoughts or opinions.

These would definitely break if run out of order. This is the path that the bootstrap repo takes, but I'm totally open to other opinions! https://github.com/twbs/bootstrap/blob/master/package.json#L24

Also my overarching plan is to get rid of style:rename, style:less and style:assets in the future.

I think these are okay to leave for now since this PR is about achieving parity. It might be a nice follow-up task to optimize all this once it's all done.

greg-a-smith · 2019-04-12T15:04:21Z

scripts/copy-assets.sh

+
+for file in scss/icons/*.{eot,svg,ttf,woff}; do cp "$file" dist; done
+
+for file in scss/fonts/72/*.{eot,svg,ttf,woff,woff2}; do cp "$file" dist/fonts/72/; done


Would there ever be other file extensions we'd want to move? Or maybe better asked ... are there certain extensions we are omitting such that we can't just copy all the file from one directory to the other?

Each of these has some .scss files that we want to omit

greg-a-smith · 2019-04-12T15:04:26Z

scripts/copy-assets.sh

+
+mkdir -p -- dist/{fonts,images} dist/fonts/72
+
+for file in scss/icons/*.{eot,svg,ttf,woff}; do cp "$file" dist; done


Would there ever be other file extensions we'd want to move? Or maybe better asked ... are there certain extensions we are omitting such that we can't just copy all the file from one directory to the other?

jbadan · 2019-04-12T15:45:17Z

@greg-a-smith

@jbadan Is there a story (or stories) created for the 5 follow-up tasks?

We have stories in the COREUI backlog, but I haven't opened issues on the repo yet. I wanted to see how this pr went over first :)

greg-a-smith

Looks good. 🚢

netlify · 2019-04-15T16:55:06Z

Deploy preview for fundamental ready!

Built with commit 42f2cf9

https://deploy-preview-1385--fundamental.netlify.com

jbadan added 10 commits April 8, 2019 08:02

renames

d2b9435

updates:

b814f96

Merge branch 'master' into fix/dist-pipline

98b7142

dist2 working state

ec0e49c

no diffs

b684ce2

delete uneeded files

d59d309

reorder scripts

2d4278b

package changes

70d0e8f

revert uneeded changes

8eb40f4

remove uneeded script

3e9ff03

quiet node-sass

c99dd69

jbadan closed this Apr 10, 2019

move cleancss to postcss task

146f681

jbadan reopened this Apr 10, 2019

jbadan changed the title ~~fix: remove gulp from dist folder build pipeline~~ DRAFT (not ready for review): fix: remove gulp from dist folder build pipeline Apr 10, 2019

jbadan added 3 commits April 11, 2019 08:29

fix broken glob

b516d7a

testing with only 1 icon file

5269741

move to asset copy to scripts

3af34e2

jbadan changed the title ~~DRAFT (not ready for review): fix: remove gulp from dist folder build pipeline~~ fix: remove gulp from dist folder build pipeline Apr 11, 2019

jbadan requested a review from a team April 11, 2019 20:48

jbadan commented Apr 11, 2019

View reviewed changes

greg-a-smith reviewed Apr 12, 2019

View reviewed changes

rename build:all to build:prod:

b14b220

greg-a-smith approved these changes Apr 12, 2019

View reviewed changes

empty commit to trigger netlify status check

42f2cf9

jbadan merged commit 2ae183a into master Apr 15, 2019

jbadan deleted the fix/dist-pipline branch April 15, 2019 18:10

droshev pushed a commit that referenced this pull request Jul 16, 2020

fix: remove gulp from dist folder build pipeline (#1385)

381b26c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove gulp from dist folder build pipeline #1385

fix: remove gulp from dist folder build pipeline #1385

jbadan commented Apr 10, 2019 •

edited

Loading

joseegm commented Apr 10, 2019 •

edited

Loading

jbadan Apr 11, 2019

greg-a-smith commented Apr 12, 2019

greg-a-smith Apr 12, 2019

jbadan Apr 12, 2019

jbadan Apr 12, 2019

greg-a-smith Apr 12, 2019

greg-a-smith Apr 12, 2019

jbadan Apr 12, 2019

greg-a-smith Apr 12, 2019

jbadan commented Apr 12, 2019

greg-a-smith left a comment

netlify bot commented Apr 15, 2019

		@@ -51,71 +41,5 @@ const sassTask = () => {
		};
		gulp.task('pkg-sass', sassTask);


		for file in scss/icons/*.{eot,svg,ttf,woff}; do cp "$file" dist; done

		for file in scss/fonts/72/*.{eot,svg,ttf,woff,woff2}; do cp "$file" dist/fonts/72/; done


		mkdir -p -- dist/{fonts,images} dist/fonts/72

		for file in scss/icons/*.{eot,svg,ttf,woff}; do cp "$file" dist; done

fix: remove gulp from dist folder build pipeline #1385

fix: remove gulp from dist folder build pipeline #1385

Conversation

jbadan commented Apr 10, 2019 • edited Loading

joseegm commented Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

greg-a-smith commented Apr 12, 2019

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

jbadan commented Apr 12, 2019

greg-a-smith left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 15, 2019

jbadan commented Apr 10, 2019 •

edited

Loading

joseegm commented Apr 10, 2019 •

edited

Loading