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.

Sign up for GitHub

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

Jump to bottom

@kanaabe => Replace Browserify with Webpack #1785

Merged
merged 25 commits into from
Jan 23, 2018
Merged

@kanaabe => Replace Browserify with Webpack #1785

merged 25 commits into from
Jan 23, 2018

Conversation

damassi
Copy link
Member

@damassi damassi commented Sep 21, 2017 •
edited
Loading

This brings Webpack into the mix, following Positron, Prediction, Volt, etc.

TODO:

  • Add yarn script for yarn link @artsy/reaction-force && yarn start
  • Add Prettier in separate PR
  • Look into Sentry Webpack sourcemap plugin in separate PR
  • Ensure babel-plugin-styled-components works properly in DEV
  • Look in to waypoint issue in /about: Uncaught Error: No handler option passed to Waypoint constructor
  • Look into @kanaabe => Replace Browserify with Webpack  #1785 (comment)
  • Hot reloading
  • Hard reloads when HMR fails or isn't applied
  • CoffeeScript
  • Jade templates
  • SVG Icons
  • Minification
  • Separate vendor bundle
  • Tests
  • Fix old global jQuery libraries

output

Other Additions

  • babel-preset-env for auto-prefixing new ES6+ features. Currently targets last to versions of each browser, down to IE10.

@damassi damassi self-assigned this Sep 21, 2017
@damassi damassi force-pushed the webpack-migration branch 5 times, most recently from de9b28d to 1dbe6c7 Compare September 21, 2017 03:29
damassi
damassi commented Sep 21, 2017
View reviewed changes
scripts/start.sh Outdated
@@ -2,4 +2,4 @@

set -e -x

forever -c 'node -r dotenv/config --max_old_space_size=1024' .
forever -c 'node -r dotenv/config --max_old_space_size=1024' . --colors
Copy link
Member Author

@damassi damassi Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

forever eats up aspects of sub-dep stdout, hence the new dev task

@damassi damassi force-pushed the webpack-migration branch 15 times, most recently from aeb96e1 to 6aaba00 Compare September 22, 2017 08:09
damassi
damassi commented Sep 22, 2017
View reviewed changes
desktop/components/main_layout/templates/scripts.jade Outdated
@@ -115,15 +115,14 @@ if sharify
!= sharify.script()

//- Add Google's jQuery
script( src="//cdnjs.cloudflare.com/ajax/libs/jquery/2.1.4/jquery.min.js" )
//- script( src="//cdnjs.cloudflare.com/ajax/libs/jquery/2.1.4/jquery.min.js" )
Copy link
Member Author

Choose a reason for hiding this comment

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

@craigspaeth - any idea why this is here? We're bundling jQuery into common.js below

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be irrelevant now, but pre-jQuery supporting common js we mixed CDN dependencies with modules. I believe there's plenty of code around Force that still depends on a global $ object, so if we remove this let's make sure to expose it via window.$ = window.jQuery = require('jquery') around here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This bit here should take care of that: https://github.com/artsy/force/pull/1785/files#diff-11e9f7f953edc64ba14b0cc350ae7b9dR86

Copy link
Contributor

@craigspaeth craigspaeth Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Nice, works for me!

@damassi damassi force-pushed the webpack-migration branch 2 times, most recently from 091184f to 6aaba00 Compare September 22, 2017 17:44
@damassi damassi force-pushed the webpack-migration branch from 6aaba00 to 84449f6 Compare October 6, 2017 01:40
@damassi damassi force-pushed the webpack-migration branch 2 times, most recently from 58820a3 to 33bce14 Compare October 6, 2017 02:22
@damassi
Copy link
Member Author

damassi commented Jan 21, 2018

Ok @kanaabe - I believe this is done! Was able to fix another styled-components issue where the displayLabel wasn't showing up in console.

Would you mind pulling down again and giving it another good spin, including mobile?

@kanaabe
Copy link
Contributor

kanaabe commented Jan 22, 2018

Desktop looks solid but noticed a couple things on mobile.

Article Impressions on: http://localhost:5000/article/artsy-editorial-17-emerging-artists-to-watch-in-2017

image

Inquiry: http://localhost:5000/inquiry/nerhol-01-slash-6-dot-7-2015

image

Error in imagesLoaded on /articles. This one isn't major so feel free to punt if you want but it basically stops the carousel from working:
image

@damassi
Copy link
Member Author

damassi commented Jan 23, 2018

Ugh I keep chasing things that I thought were fixed; one sec!

@damassi
Copy link
Member Author

damassi commented Jan 23, 2018

@kanaabe - this is 🍏 (finalllly) - Also, caught an unrelated bug in artsy/reaction#494 while testing so once that's merged I'll bump reaction version and update in force.

@orta
Copy link
Contributor

orta commented Jan 23, 2018

📗

@kanaabe kanaabe merged commit 5d4f0e2 into artsy:master Jan 23, 2018
@damassi damassi deleted the webpack-migration branch January 23, 2018 22:49
@damassi damassi mentioned this pull request Jan 23, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craigspaeth craigspaeth craigspaeth left review comments

@alloy alloy alloy left review comments

Assignees

@kanaabe kanaabe

Labels
Enhancement Maintenance Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@damassi @alloy @craigspaeth @kanaabe @orta