Skip to content

Enhance the webpack-encore integration #587

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.

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

Closed
wants to merge 5 commits into from

Conversation

alOneh
Copy link
Contributor

@alOneh alOneh commented Jun 13, 2017

This enhance the first integration (#586) of webpack-encore.

Fixes :

  • jquery issue
  • typeahead issue
  • delete modal issue
  • Add build sources
  • Update webpack-encore version

@@ -5,6 +5,11 @@ Encore
.setPublicPath('/build')
.cleanupOutputBeforeBuild()
.autoProvidejQuery()
.autoProvideVariables({
"window.jQuery": "jquery",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a PR on webpack-encore to provide this automatically

@javiereguiluz
Copy link
Member

Thanks a lot for working on this! I've tested your PR in the page where most JS is used (/app_dev.php/en/admin/post/1/edit) and:

  • The "Delete post" modal works
  • The datepicker works too
  • I don't see any error in the browser console

However, the following doesn't work yet:

  • The label autocompletion
  • The code syntax highlight (the modal window appears, but the code shows no color)

@alOneh
Copy link
Contributor Author

alOneh commented Jun 13, 2017 via email

@alOneh
Copy link
Contributor Author

alOneh commented Jun 13, 2017

@javiereguiluz all issues have been addressed

@javiereguiluz
Copy link
Member

I've reviewed your latest changes. Everything is indeed fixed. Thanks a lot!


Now I have 3 comments:

  1. Why "import" instead of "require" ? PHPStorm is showing some warning messages too:

js-error

If "import" is the way to go, we should update Encore docs too.

  1. I don't remember if the autocomplete feature worked this way before ... but right now it only autocompletes de already existing tags of the post. This is not very intuitive. @yceruto maybe can confirm if this is the expected behavior:

label-autocomplete

  1. The file sizes of the assets generated for production seems OK ... except for app.js ... which weights more than 600 KB because it includes the highlighting code for ALL languages (although you configured only PHP and Twig ... this happened to me too).

app-js-size

@alOneh
Copy link
Contributor Author

alOneh commented Jun 13, 2017

1/ As we are in ES6 syntax we can use import, require is a ES2015 syntax to load modules, if you inspect your bundle you will see that webpack (due to Babel) transform those import by require. To fix the IDE issue you can switch the syntax to ES6 to get rid of warnings.

3/ I drop the app.js size to 2 ko
capture d ecran 2017-06-13 a 18 06 29

@yceruto
Copy link
Member

yceruto commented Jun 13, 2017

@javiereguiluz AFAIK the expected autocomplete list will be all available tags (used or not) and you can also insert new ones (typing it there + enter). Just a few tags we have by default :)

[...] This is not very intuitive [...]

I'm not sure what do you mean?

@javiereguiluz
Copy link
Member

@alOneh I've tested your last changes and everything is great! To me this PR is now ready to be merged.

The only optional change I'd like to do is to get rid of the moment.js library. It's humongous and it makes our admin.js to weight almost 400 KB 😱

@yceruto never mind, let's keep it as is and we'll tweak it in the future if needed. Thanks!

@stof
Copy link
Member

stof commented Jun 14, 2017

@alOneh does moment has a way to skip the import of the part loading locales dynamically, as you did for highlight.js ? This would avoid bundling all moment locales if we don't need them.

@alOneh
Copy link
Contributor Author

alOneh commented Jun 14, 2017

@stof yes we can, I made a fix and after that the admin.js size is ~180 KB.
capture d ecran 2017-06-14 a 11 10 35

It can be ugly but as webpack config is an object we can do that even if it will be nice if we can add this kind of feature to the webpack-encore API. I'll make a proposal later.

@javiereguiluz
Copy link
Member

Tested again and it works perfectly. This is now finished, so I'm merging it and later, if needed, @weaverryan can make any tweak to this config before releasing the new version of the Symfony Demo. Thanks @alOneh!

@javiereguiluz javiereguiluz changed the title WIP: Enhance the webpack-encore integration Enhance the webpack-encore integration Jun 14, 2017
@alOneh
Copy link
Contributor Author

alOneh commented Jun 14, 2017

@javiereguiluz I don't push build files !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants