WIP: Initial Webpack work #1306

joshhunt · 2016-12-30T06:19:26Z

As has been discussed over in #869, this represents the bulk of the initial work in getting Webpack up and running for bundling all of DIM.

Run it (at the moment) with npm start -s -- --watch. Everything should be mostly working (at least, everything looks alright. I can't transfer items atm due to a OriginHeaderDoesNotMatchKey error, I'm not sure if that's just Bungie updating slowly or an error I've introduced).

Note that I've updated the chrome manifest.json to point to the new generated/index.html, so you'll need to reload the extension.

We have all of the dependencies and application scripts bundle up into one JS file, as well as getting Webpack to manage styles and some static assets.

TODO:

Import external CSS (Hotkeys and Font Awesome). I guess we can either use CSS Loader + ExtractTextPlugin (so similar to how the Sass is built), or @import them in the main.scss file.
Sort out the random images that arent referenced correctly. Just using file-loader + const spartanUrl = require('./images/spartan.png') might be appropriate.
Create a proper 'watch' mode for development
Clean out the generated build so it doesnt keep accumulating with hashed files
Ensure 'production' build (minification) is working
- Create new package.json script commands to make this easy to run
Understand the Chrome Extension deployment process and make sure that's all working properly.
Document new build and commands to run during development and production.

houndci-bot · 2016-12-30T06:19:44Z

app/scripts/services/dimManifestService.factory.js

@@ -197,7 +200,7 @@
    function unzipManifest(blob) {
      return $q(function(resolve, reject) {
        zip.useWebWorkers = true;
-        zip.workerScriptsPath = "vendor/zip.js/WebContent/";
+        zip.workerScriptsPath = 'zipjs/';


'zip' is not defined no-undef

houndci-bot · 2016-12-30T06:19:44Z

app/scripts/services/dimManifestService.factory.js

@@ -197,7 +200,7 @@
    function unzipManifest(blob) {
      return $q(function(resolve, reject) {
        zip.useWebWorkers = true;
-        zip.workerScriptsPath = "vendor/zip.js/WebContent/";
+        zip.workerScriptsPath = 'zipjs/';


'zip' is not defined no-undef

joshhunt · 2016-12-30T06:24:06Z

'Longer term' (as in it's probably not needed to merge this into master) we would want to understand how and where all the external dependencies are used and remove that ugly block of window._ = require('underscore').. at the top of app/index.js.

Also now that everything is seperate modules, we can remove the IIFE and "use strict" around all the files.

bhollis · 2016-12-30T06:31:56Z

Wonderful!

What would it take to plug babel into the process so we can use babili and ng-annotate?

joshhunt · 2016-12-30T06:44:19Z

Babel is already operating over all the javascripts, so any additional plugins should be fairly easy.

I'm not sure how Babili would go though. Last time I used it (a while ago) I had much difficulty getting it to do anything, so we can see how that goes I guess 😄

SunburnedGoose · 2016-12-30T07:55:15Z

delphiactual · 2016-12-30T08:00:32Z

npm start -s -- --watch

joshhunt · 2016-12-30T10:33:58Z

🤔 just plain npm start should work - it does for me. I'm wondering if it's just a Windows issue... I might give it a spin in a Windows VM and see how it goes.

delphiactual · 2016-12-30T10:53:52Z

npm install generates the following errors on Windows

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.0.0 (node_modules\chokidar\node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.0.15: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})
npm WARN extract-text-webpack-plugin@2.0.0-beta.4 requires a peer of webpack@^2.1.0-beta.19 but none was installed.
npm WARN html-webpack-plugin@2.24.1 requires a peer of webpack@1 || ^2.1.0-beta but none was installed.

delphiactual · 2016-12-30T11:10:58Z

@joshhunt This is what I had to do to get it to work in Windows...

delphiactual@333d21d

EDIT: styles are not working but npm start completed

joshhunt · 2016-12-30T11:14:31Z

The webpack.js change I guess would be due to different versions of node that were running locally.

Dropping down to a prev webpack version though is quite interesting though. I'll take at a look at them all and see what we can do.

delphiactual · 2016-12-30T11:29:09Z

Yep, I was running an older version of node... 4.4.4 upgraded and reverted the changes to webpack.js and all is well there...

delphiactual · 2016-12-30T11:30:01Z

@SunburnedGoose you forgot to npm install

delphiactual · 2016-12-30T11:45:15Z

@joshhunt delphiactual@d73a87a fixed it for me they have not pushed new builds out yet that have the rc support but they are in their master branches

SunburnedGoose · 2016-12-30T18:48:51Z

I ran install. I'll delete the folder.

delphiactual · 2016-12-30T19:27:20Z

@joshhunt sent you a PR that loads font-awesome and gets webpack to work on windows.

joshhunt · 2016-12-30T21:51:30Z

@delphiactual 💃 great stuff!

@SunburnedGoose that exit status code is a bit funky. Looks like it might be related to sass/node-sass#1283 or npm/npm#11024. does npm remove node-sass; npm install node-sass work file for you?

joshhunt · 2016-12-30T23:00:49Z

What is people's thoughts on the best way for generating the complete build, in regards to loading it up for the Chrome Extension? You'll note at the moment that the options url (and the url for DIM I guess) is now chrome-extension:/.../generated/index.html - Ideally this URL should be the same as before.

I'm thinking to utilise CopyWebpackPlugin and copy the manifest (and other supporting files, like icons and whatnot) into the app/generated folder and instructing developers to point the unpack Chrome extension to there. Personally, I think it would be somewhat nicer and cleaner to utilise that dist/chrome folder for this (and remove it from git with .gitignore).

I'm not 100% across how all this Chrome stuff works, so I'll have to defer to the expertise of others here.

Load fontawesome + Better Windows support?

houndci-bot · 2016-12-31T00:01:20Z

app/scripts/minmax/dimMinMax.controller.js

                              }
                            },
                            setHash: 0
                          };
+                          console.log({set});


A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

houndci-bot · 2016-12-31T00:01:21Z

app/scripts/minmax/dimMinMax.controller.js

                              }
                            },
                            setHash: 0
                          };
+                          console.log({set});


A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

delphiactual · 2016-12-31T00:02:13Z

I did have to run npm rebuild node-sass after upgrading node

joshhunt · 2016-12-31T00:10:11Z

app/scripts/minmax/dimMinMax.controller.js

                              },
                              STAT_STRENGTH: {
                                value: 0,
                                tier: 0,
                                name: 'Strength',
-                                icon: 'images/Strength.png'
+                                icon: require('app/images/Strength.png'),


I'm trying to get these images working

But I don't really know where they're defined... This was my first guess, but this code doesn't appear to be executed when rendering the inventory page. console.log and breakpoints arent hit. Any idea where/how those images are being referenced?

https://github.com/DestinyItemManager/DIM/blob/dev/app/scripts/services/dimStoreService.factory.js#L1563

the code you changed is just for in LoadoutBuilder

ahh - that's why find wasnt working ;)

require stat icons

… through html-loader

joshhunt · 2016-12-31T02:06:56Z

What is people's thoughts on the best way for generating the complete build, in regards to loading it up for the Chrome Extension?...

Hmm... In thinking about this more, perhaps requiring developers to re-add the chrome extension isnt the best approach here (its a bit annoying, and would require updating the Bungie API origin header). Ideally, I would prefer to build into dist/

What if we instead rename app/ to src/ and build into app (and ensure that isnt comitted into git)? The naming would be a bit unconventional but it would ease the migration for existing developers.

Or are we okay with the one-time only pain of having to readd the extension and updating API settings?

SunburnedGoose · 2016-12-31T02:08:53Z

Origin header takes a wildcard * btw.

SunburnedGoose · 2016-12-31T03:17:51Z

@joshhunt After some googling, it seems that error was a regression in node v7.1 which is what I had :/

Updated to v7.3 and it works.

joshhunt · 2016-12-31T04:25:33Z

Ahh neat - I'm glad to hear that.

I'm pretty happy with the work that's here - I think this is at least at a state where it can go into DIM/webpack, if not dev.

The only thing left I think is to make sure that the packaging for Chrome Webstore is all good. Don't really know much about that one...

There's more that can be done, but I think this represents a pretty good 'MVP'

SunburnedGoose

Does this differentiate between iOS Safari and Desktop Safari when supporting the last two versions? Just curious about the addition and removal of the Safari filter.

joshhunt · 2016-12-31T04:41:29Z

Honestly, it was only there because I copied the config from the babel-present-env readme, and then I just removed it because I thought prev 2 versions is good enough. I'm indifferent as to what's actually used.

'Safari' is just for desktop. iOS is for iOS. See browserlist.

SunburnedGoose · 2016-12-31T04:43:53Z

Looks great and works fine on my end.

joshhunt added 6 commits December 30, 2016 14:49

Remove generated files from Webpack

c3c3ef4

Prevent generated dir from being commited back into git

53fd612

Initial Webpack configuration. App not running correctly yet

02efea1

Revert back to underscore, get more deps working including ZipJS

e7bcdc2

Got styles working

9a453c1

Hash css file

786e864

houndci-bot reviewed Dec 30, 2016

View reviewed changes

Update .babelrc

69dd6d1

delphiactual added 2 commits December 30, 2016 04:08

windows patch webpack

333d21d

whitespace fix

770fb63

delphiactual added 2 commits December 30, 2016 04:32

revert webpack.js

98f7a9c

workaround for webpack-2-rc

d73a87a

font-awesome loaded

4bc3bc6

joshhunt and others added 3 commits December 31, 2016 10:36

Merge pull request #1 from delphiactual/patch-1

a326a28

Load fontawesome + Better Windows support?

Load external CSS from node_modules

e547f74

Better names for hashed assets, added app/ as alias for root of source

9f8f305

houndci-bot reviewed Dec 31, 2016

View reviewed changes

Load stat icons using require() (but its not working 🤔)

c9fc8c9

joshhunt force-pushed the webpack branch from afc7e6d to c9fc8c9 Compare December 31, 2016 00:08

joshhunt commented Dec 31, 2016

View reviewed changes

delphiactual and others added 4 commits December 30, 2016 17:27

require icons

6f01a1a

Merge pull request #2 from delphiactual/patch-1

f213f07

require stat icons

Split out dimLoadout.directive template into external file to load it…

3b8cd02

… through html-loader

Silence hound about global Zip module

b7a8b60

Update template to use i18n

e22494a

joshhunt added 4 commits December 31, 2016 13:19

Tidy up build dir for webpack assets

876c2a1

Load views using the new webpack config

d50ffc6

Update Zip.js worker dir

6e85be2

Lowercase attribute image filenames

21eb8b3

joshhunt added 4 commits December 31, 2016 14:45

Better Prod and Dev webpack modes, with NPM scripts

a599fef

Update .babelrc to remove the safari version

d638adf

Clean up globals in eslintrc and add ES6 env

f74211d

Remove a whole bunch of (now) unused Grunt scripts

21b44f2

SunburnedGoose reviewed Dec 31, 2016

View reviewed changes

SunburnedGoose merged commit 65f31a1 into DestinyItemManager:webpack Dec 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Initial Webpack work #1306

WIP: Initial Webpack work #1306

joshhunt commented Dec 30, 2016 •

edited

Loading

houndci-bot Dec 30, 2016

houndci-bot Dec 30, 2016

joshhunt Dec 30, 2016

joshhunt commented Dec 30, 2016 •

edited

Loading

bhollis commented Dec 30, 2016

joshhunt commented Dec 30, 2016

SunburnedGoose commented Dec 30, 2016

delphiactual commented Dec 30, 2016

joshhunt commented Dec 30, 2016

delphiactual commented Dec 30, 2016 •

edited

Loading

delphiactual commented Dec 30, 2016 •

edited

Loading

joshhunt commented Dec 30, 2016

delphiactual commented Dec 30, 2016

delphiactual commented Dec 30, 2016

delphiactual commented Dec 30, 2016

SunburnedGoose commented Dec 30, 2016

delphiactual commented Dec 30, 2016

joshhunt commented Dec 30, 2016 •

edited

Loading

joshhunt commented Dec 30, 2016

houndci-bot Dec 31, 2016

houndci-bot Dec 31, 2016

delphiactual commented Dec 31, 2016

joshhunt Dec 31, 2016

delphiactual Dec 31, 2016

delphiactual Dec 31, 2016

delphiactual Dec 31, 2016

joshhunt Dec 31, 2016

joshhunt commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

joshhunt commented Dec 31, 2016 •

edited

Loading

SunburnedGoose left a comment

joshhunt commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

WIP: Initial Webpack work #1306

WIP: Initial Webpack work #1306

Conversation

joshhunt commented Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshhunt commented Dec 30, 2016 • edited Loading

bhollis commented Dec 30, 2016

joshhunt commented Dec 30, 2016

SunburnedGoose commented Dec 30, 2016

delphiactual commented Dec 30, 2016

joshhunt commented Dec 30, 2016

delphiactual commented Dec 30, 2016 • edited Loading

delphiactual commented Dec 30, 2016 • edited Loading

joshhunt commented Dec 30, 2016

delphiactual commented Dec 30, 2016

delphiactual commented Dec 30, 2016

delphiactual commented Dec 30, 2016

SunburnedGoose commented Dec 30, 2016

delphiactual commented Dec 30, 2016

joshhunt commented Dec 30, 2016 • edited Loading

joshhunt commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delphiactual commented Dec 31, 2016

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

joshhunt commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

joshhunt commented Dec 31, 2016 • edited Loading

SunburnedGoose left a comment

Choose a reason for hiding this comment

joshhunt commented Dec 31, 2016

SunburnedGoose commented Dec 31, 2016

joshhunt commented Dec 30, 2016 •

edited

Loading

joshhunt commented Dec 30, 2016 •

edited

Loading

delphiactual commented Dec 30, 2016 •

edited

Loading

delphiactual commented Dec 30, 2016 •

edited

Loading

joshhunt commented Dec 30, 2016 •

edited

Loading

joshhunt commented Dec 31, 2016 •

edited

Loading