Use Webpack Encore to manage assets #586

javiereguiluz · 2017-06-12T17:47:15Z

Most of the features are working ... but I'm having troubles with the autocompletion of the lables. It looks horrible and it doesn't autocomplete anything:

I know that the admin.js committed in this PR is wrong, but I just wanted to show the last I tried. I've spent most of this morning trying to make this work 😢

Maybe @yceruto, who added this feature originally, or @weaverryan, who created Webpack Encore, can help us here. Thanks!

javiereguiluz · 2017-06-12T18:02:23Z

When I try to compile the production assets, I get this error:

$ ./node_modules/.bin/encore production
Running webpack ...

 ERROR  Failed to compile with 8 errors                                                                                                                                                                                      7:59:36 PM

These relative modules were not found:

* ../fonts/lato-normal/lato-normal.woff2 in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-normal/lato-normal.woff in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-normal-italic/lato-normal-italic.woff2 in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-normal-italic/lato-normal-italic.woff in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-bold/lato-bold.woff2 in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-bold/lato-bold.woff in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-bold-italic/lato-bold-italic.woff2 in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss
* ../fonts/lato-bold-italic/lato-bold-italic.woff in ./~/css-loader!./~/resolve-url-loader!./~/sass-loader/lib/loader.js?{"sourceMap":true}!./app/Resources/assets/scss/app.scss

The previous spot is an outdated spot anyways, and it caused an edge-case bug with resolution of some SASS paths: symfony/demo#586

weaverryan · 2017-06-12T19:12:25Z

See symfony/webpack-encore#4 for production error issue :)

weaverryan · 2017-06-12T19:34:21Z

About the tags thing, before, we had this file: https://github.com/symfony/symfony-demo/blob/master/web/css/bootstrap-tagsinput.css

But this file was deleted, and is not being loaded in the new setup. You do have this in main.scss:

@import "~bootstrap-tagsinput/src/bootstrap-tagsinput.css";

But that doesn't have the same contents as the old bootstrap-tagsinput.css. If I bring back the old contents, it "looks" correctly.

There is some other problem too, as even with this, I don't see the old tags. There's no JS error, but for some reason the autocomplete box thinks that there is never any results (I can see the class tt-empty in the DOM)

weaverryan · 2017-06-12T19:49:54Z

Production issue should be fixed in 0.7.2

yceruto · 2017-06-12T22:34:42Z

CSS
The actual web/css/bootstrap-tagsinput.css (it includes typeahead style) is a custom stylesheet adapted to current demo style (e.g. FontAwesome), so I suggest you use it as is, for now. Later we would include the originals overwriting the relevant classes.

JS
Try this:

  var source = new Bloodhound({
      local: $input.data('tags'),
      queryTokenizer: Bloodhound.tokenizers.whitespace,
      datumTokenizer: Bloodhound.tokenizers.whitespace
  });
+ source.initialize();
  $input.tagsinput({
      trimValue: true,
      focusClass: 'focus',
      typeaheadjs: {
          name: 'tags',
-         source: source
+         source: source.ttAdapter()
      }
  });

weaverryan

Awesome! I'm sure we'll need to work on a few of the more complex lines that I'm saying aren't needed :). That's great, because it will serve as a wonderful, real-world example.

Thanks!

weaverryan · 2017-06-13T00:01:32Z

app/Resources/assets/js/admin.js

@@ -0,0 +1,69 @@
+var $ = require('jquery');


What about putting the assets directory at the root of the project? It's a bit more "flex" like

Yes ... but let's do that when we switch the entire app to Flex.

weaverryan · 2017-06-13T00:03:19Z

app/Resources/assets/js/admin.js

@@ -0,0 +1,69 @@
+var $ = require('jquery');
+window.$ = $;
+window.jQuery = $;


You shouldn't need either of these unless we still have some JavaScript code in a template somewhere that is relying on these to be globally available. The only place I could see was login.html.twig. We could move that little bit of JavaScript here, or into a new login entry as an example of a page-specific JS file.

If I remove it, I see this error in any backend page:

admin.js:38009 Uncaught TypeError: Cannot read property 'fn' of undefined at admin.js:38009 at Object.<anonymous> (admin.js:38134) at __webpack_require__ (admin.js:20) at Object.<anonymous> (admin.js:35021) at __webpack_require__ (admin.js:20) at Object.<anonymous> (admin.js:42417) at __webpack_require__ (admin.js:20) at module.exports.ctor.super_ (admin.js:66) at admin.js:69

weaverryan · 2017-06-13T00:04:11Z

app/Resources/assets/js/admin.js

+window.$ = $;
+window.jQuery = $;
+
+require('bootstrap-sass');


How about a comment above this:

// loads the Bootstrap jQuery plugins

weaverryan · 2017-06-13T00:08:02Z

app/Resources/assets/js/admin.js

+window.jQuery = $;
+
+require('bootstrap-sass');
+require('moment');


This shouldn't be needed. I think only the eonasdan-bootstrap-datetimepicker library uses it, and it should (and appears to) require moment internally on its own https://github.com/Eonasdan/bootstrap-datetimepicker/blob/25c11d79e614bc6463a87c3dd9cbf8280422e006/src/js/bootstrap-datetimepicker.js#L42

weaverryan · 2017-06-13T00:09:57Z

app/Resources/assets/js/admin.js

+
+require('imports-loader?define=>false!typeahead.js/dist/typeahead.jquery.min.js');
+const Bloodhound = require('imports-loader?define=>false!typeahead.js/dist/bloodhound.js');
+window.Bloodhound = Bloodhound;


Again, this shouldn't be needed (in theory), but it's possible there's some legacy library that isn't requiring this correctly. We'll need to make sure each line is needed, and document which lines are needed and why

If I remove those, I see this error:

Uncaught ReferenceError: Bloodhound is not defined at HTMLDocument.<anonymous> (admin.js:35043) at mightThrow (admin.js:11908) at process (admin.js:11976)

I see a lot of errors related to this library. Maybe we can replace it with some simpler alternative library? After all, our needs are super basic.

add something like .autoProvideVariables({ 'window.Bloodhound': "bloodhound") to your webpack.config.js

We might decide to change: twitter/typeahead.js#1618

weaverryan · 2017-06-13T00:11:28Z

app/Resources/assets/js/app.js

+var hljs = require('highlight.js');
+hljs.registerLanguage('php', require('highlight.js/lib/languages/php'));
+hljs.registerLanguage('twig', require('highlight.js/lib/languages/twig'));
+window.hljs = hljs;


This shouldn't be needed

weaverryan · 2017-06-13T00:11:37Z

app/Resources/assets/js/app.js

+hljs.registerLanguage('php', require('highlight.js/lib/languages/php'));
+hljs.registerLanguage('twig', require('highlight.js/lib/languages/twig'));
+window.hljs = hljs;
+hljs.initHighlightingOnLoad();


does this need to be in a document.ready?

weaverryan · 2017-06-13T00:13:31Z

app/Resources/assets/scss/admin.scss

@@ -0,0 +1,26 @@
+@import "~bootswatch/flatly/variables";
+@import "~eonasdan-bootstrap-datetimepicker/src/sass/bootstrap-datetimepicker-build.scss";
+@import "~bootstrap-tagsinput/src/bootstrap-tagsinput.css";


When a CSS file is a dependency of some JS library, I usually like to require the CSS file right in our .js file, right next to where I require the related JS file. Later, if we stopped needing the bootstrap-datetimepicker JS file, we would also remove the CSS require right next to it.

weaverryan · 2017-06-13T00:15:37Z

package.json

+    "lato-font": "^3.0.0",
+    "moment": "^2.18.1",
+    "typeahead.js": "^0.11.1"
+  }


These should all go into devDependencies. It doesn't really matter, but the idea is that you really only need all of this stuff while your "developing". You won't need these files on production, because you will have already built your final files and are shipping those statically.

@weaverryan it depends how you deploy actually. When deploying on heroku, the build happens on the heroku build server, and it does not include dev deps

weaverryan · 2017-06-13T00:16:04Z

webpack.config.js

+Encore
+    .setOutputPath('web/build/')
+    .setPublicPath('/build')
+    .setManifestKeyPrefix('build')


We shouldn't need this line - but if you do, let me know :)

stof · 2017-06-13T08:50:06Z

.gitignore

@@ -1,5 +1,7 @@
 /app/config/parameters.yml
 /build/
+!/web/build/


what is this rule about ? AFAIK, it does not do anything, as the folder is not excluded before (/build/ does not match it AFAIK, unlike build/ would do)

stof · 2017-06-13T08:52:12Z

app/Resources/assets/js/admin.js

+window.Bloodhound = Bloodhound;
+require('bootstrap-tagsinput');
+
+$(document).ready(function () {


this should be changed to just $(function() {...}) IMO, which is the proper jQuery API (no need to create a jQuery collection for the document, while .ready() does not care about the collection)

stof · 2017-06-13T09:05:44Z

app/Resources/assets/scss/app.scss

-    -webkit-flex-direction: column;
-    flex-direction: column;
-    min-height: 100vh
+  display: -webkit-flex;


could we use autoprefixer instead ?

I propose to remove those instead. They are unneeded now.

yeah, we could consider that we target modern browsers only as we target developers. But I would still consider it useful to demo how autoprefixer can be used (many sites will actually need it)

stof · 2017-06-13T09:33:10Z

app/Resources/assets/js/admin.js

+require('moment');
+require('eonasdan-bootstrap-datetimepicker');
+
+require('imports-loader?define=>false!typeahead.js/dist/typeahead.jquery.min.js');


why using the imports-loader ? The dist files are using UMD, so they should work fine with CommonJS requirements.

I think all your issues about defining global variables are actually related to the weird loading you do here.

I have no idea ... I spent a lot of time trying to make typeahead/bloodhound work. Nothing worked. This was one of the tests that I did after reading some of the issues reported about this library (there are like a lot of issues reported!)

javiereguiluz · 2017-06-13T10:12:56Z

I'm going to merge this "as is" although it's broken. That will allow others take over this and fix my mistakes. I removed all the window.$ = $; window.jQuery = $ and now most features are broken (probably my fault!).

List of things to fix:

Typeahead library is dead ... use a fork instead (Algolia probably is the most reassuring one)
The bootstrap datepicker doesn't work (it worked before removing the jQuery thing)
The highlighting doesn't work (it worked before ... but it generated a 600KB app.js because it includes ALL the highlighting languages, instead of just the PHP and Twig we need ... I tried to reduce the file size ... but now it doesn't work)
The modal window of "Show code" works ... but the modal window of "Delete post" doesn't work. It worked yesterday (so maybe again is the jQuery thing).

@alOneh knows all this well and he's very skilled with Webpack (he even updated the Symfony Demo to use pure Webpack, without Encore. See: https://github.com/alOneh/sf-live-2017-symfony-webpack/blob/master/webpack.config.js) So maybe this can be fixed between @weaverryan and @alOneh

stof · 2017-06-13T12:06:29Z

@javiereguiluz the issue with moment locales and syntax highlighting is probably the same:
webpack sees a dynamic require call loading the translation/grammar dynamically for server-side usage of the library, and so it figures out it needs to bundle all matching files to avoid breaking the client-side usage.

This PR was squashed before being merged into the master branch (closes #587). Discussion ---------- Enhance the webpack-encore integration This enhance the first integration (#586) of webpack-encore. Fixes : * jquery issue * typeahead issue * delete modal issue - [ ] Add build sources - [ ] Update webpack-encore version Commits ------- 13d02c1 Enhance the webpack-encore integration

javiereguiluz added 3 commits June 12, 2017 19:43

Use Webpack Encore to manage assets

5fbc840

Added the compiled assets to make the app work out of the box

64998f8

More work on assets

0fbf7cc

weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Jun 12, 2017

Moving minification options

812edb2

The previous spot is an outdated spot anyways, and it caused an edge-case bug with resolution of some SASS paths: symfony/demo#586

weaverryan mentioned this pull request Jun 12, 2017

Moving minification options symfony/webpack-encore#4

Merged

weaverryan reviewed Jun 13, 2017

View reviewed changes

stof requested changes Jun 13, 2017

View reviewed changes

stof reviewed Jun 13, 2017

View reviewed changes

More work on assets

fd4e4d8

javiereguiluz closed this in dc47290 Jun 13, 2017

alOneh mentioned this pull request Jun 13, 2017

Enhance the webpack-encore integration #587

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Webpack Encore to manage assets #586

Use Webpack Encore to manage assets #586

javiereguiluz commented Jun 12, 2017

javiereguiluz commented Jun 12, 2017

weaverryan commented Jun 12, 2017

weaverryan commented Jun 12, 2017

weaverryan commented Jun 12, 2017

yceruto commented Jun 12, 2017 •

edited

Loading

weaverryan left a comment

weaverryan Jun 13, 2017

javiereguiluz Jun 13, 2017

weaverryan Jun 13, 2017

javiereguiluz Jun 13, 2017

weaverryan Jun 13, 2017

weaverryan Jun 13, 2017

weaverryan Jun 13, 2017

javiereguiluz Jun 13, 2017

javiereguiluz Jun 13, 2017

alOneh Jun 13, 2017

stof Jun 13, 2017

weaverryan Jun 13, 2017

weaverryan Jun 13, 2017

weaverryan Jun 13, 2017

weaverryan Jun 13, 2017

stof Jun 13, 2017

weaverryan Jun 13, 2017

stof Jun 13, 2017

stof Jun 13, 2017

stof Jun 13, 2017

javiereguiluz Jun 13, 2017

stof Jun 13, 2017

stof Jun 13, 2017

javiereguiluz Jun 13, 2017

javiereguiluz commented Jun 13, 2017

stof commented Jun 13, 2017

Use Webpack Encore to manage assets #586

Use Webpack Encore to manage assets #586

Conversation

javiereguiluz commented Jun 12, 2017

javiereguiluz commented Jun 12, 2017

weaverryan commented Jun 12, 2017

weaverryan commented Jun 12, 2017

weaverryan commented Jun 12, 2017

yceruto commented Jun 12, 2017 • edited Loading

weaverryan left a 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

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

javiereguiluz commented Jun 13, 2017

stof commented Jun 13, 2017

yceruto commented Jun 12, 2017 •

edited

Loading