JavaScript refactoring #303

matatirosolutions · 2017-11-19T12:30:17Z

In PR #257 @bensampaio refactored the JavaScript to remove the reliance on the Google Closure library and to support using this bundle with WebPack (as identified in #264).

That pull request has stalled because the JavaScript tests were also moved from the deprecated jsUnit to the more modern Jasmine as part of the same PR.

I discussed this with @stof during #SymfonyCon2017Hackday because I wanted to see this move forward to allow for easier integration with WebPack.

This PR attempts to do that by bringing the excellent work Bruno did in refactoring the JS in without refactoring the tests as part of the same process. Essentially I have brought together the work Bruno did on updated JS and an updated JS build process, with the old tests.

It was necessary to make three minor changes to the test suite which I wish to explain to set minds at ease:

it became necessary to include goog.require('goog.structs.Map'); in the test suite because it is used by one of the tests which was relying on this having been required by the original router.js
that same test testGetRoutes() had to be modified to use expected.toObject() because it was previously running against the uncompiled code which returned a goog.structs.Map where-as we are now returning a standard object instead.
router_test.html was modified to load the compiled library, rather than the 'source' library. This was needed because the test suite expects (amongst other things) for there to be a global fos object. The updated JS code is also written in ES6 which is then transpiled as part of the build process to support backwards compatibility.

The documentation around contributing has also been updated to reflect the new build process.

…ove reliance on Google Closure and support WebPack integration

weaverryan · 2017-12-03T19:12:50Z

Hey @matatirosolutions!

I finally got a moment to look at this. Nice work! The changes are still (necessarily) HUGE, which makes them tough to review. However, because you made no changes to the test suite, I think is the most important thing.

I did notice a few file permissions changes. Unless those are on purpose, could you revert them?

@tobias-93 What are your thoughts on this? I think we should just test it manually a bit, then merge it in. If the automated tests + some manually testing doesn't find an issue, then that's the best we can do. We really need to move forward with how the JS is written in the lib.

tobias-93 · 2017-12-04T09:41:34Z

@matatirosolutions Good work! I think we can really use this going forward. For the code: I'm not a JS genius so I'll rely on the tests and do some manual testing myself in order to verify it as @weaverryan suggested.
I do, however, worry about the permissions change, I want you to revert them. Also: why did you create a copy of the router.js in the public folder? Duplicate files are a pain in the ass. Either convert it to a symlink or find a way to remove one of the two copies.

willdurand

I like what I am seeing.

willdurand · 2017-12-04T10:43:12Z

.travis.yml

@@ -22,7 +22,7 @@ before_install:

 install:
  - composer update $COMPOSER_FLAGS --prefer-dist
-  - npm install google-closure-library
+  - cd Resources && npm install google-closure-library && cd ../


I am not sure to understand why this is needed here.

The changing of directories? I was trying to keep it such that all node_module dependencies were in the Resources directory rather than some at the root of the project, and others in the Resource directory.

willdurand · 2017-12-04T10:43:47Z

Resources/doc/usage.rst

@@ -1,7 +1,7 @@
 Usage
 =====

-Add these two lines in your layout:
+In applications not using WebPack add these two lines in your layout:


I think we write "Webpack" (see: https://en.wikipedia.org/wiki/Webpack).

That would seem to be the wikipedia view, yet https://webpack.js.org/ seems to favour all lowercase, so I've gone with that - hope that's ok?

Ok for all lowercase then

This is still not conform to the discussed capitalisation

Sorry @tobias-93 fixed now!

willdurand · 2017-12-04T10:44:46Z

Resources/doc/usage.rst

+.. code-block:: javascript
+
+    const routes = require('../../web/js/fos_js_routes.json');
+    import {Router, Routing} from '../../vendor/friendsofsymfony/jsrouting-bundle/Resources/public/js/router.js';


Why are you mixing ES and Node imports?

I don't believe that I am - in this example I load the routes which have been generated as .json from fos:js-routing:dump, then import the actual Router and Routing objects for use - but perhaps I don't understand your comment?

I meant either you use require() or import ... from.

willdurand · 2017-12-04T10:45:56Z

Resources/doc/usage.rst

+    import {Router, Routing} from '../../vendor/friendsofsymfony/jsrouting-bundle/Resources/public/js/router.js';
+
+    Router.setData(routes);
+    Routing.generate('rep_log_list');


Not sure why we need two different objects here but I'll keep reading :)

BC, however based on a comment by @stof this has now been refactored and removed

willdurand · 2017-12-04T10:56:31Z

Note: I dont think the singleton pattern is correctly implemented here (with a global (module-scoped) instance returned by a method in a class of this module). I would go for a static property on the class instead.

stof · 2017-12-04T11:12:09Z

Resources/doc/usage.rst

-.. note::
-
-    If you are not using Twig, then it is no problem. What you need is to add
+.. note:: If you are not using Twig, then it is no problem. What you need is to add


this change looks wrong to me.

How so? All I did was remove two line-breaks?

and this is precisely the issue, as having some content on the same line than the directive or as an indented block following it has a totally different meaning in the rST syntax.

Ok - I must be being miss-lead by the editor I'm using - the only difference I see is that without the line-breaks the text of the note is closer to the heading of the note (and therefore to my eye looks neater ;-). I've reverted the change in a soon-to-be pushed commit.

stof · 2017-12-04T11:14:33Z

Resources/js/router.test.js

@@ -386,7 +387,7 @@ function testGetRoutes() {
        blog: 'test'
    });

-    assertObjectEquals(expected, router.getRoutes());
+    assertObjectEquals(expected.toObject(), router.getRoutes());


isn't it just the object passed as argument to new goog.structs.Map then ? If yes, it may be simpler to remove the goog Map wrapper instead.

My reasoning for doing it this way was to minimise the changes to the tests as far as possible.

Your other comments below have also simplified the install/config process to the point that it adds no additional effort and the test suite itself still relies on the Google library.

stof · 2017-12-04T11:14:58Z

Resources/package.json

+  ],
+  "files": [
+    "js/router.js",
+    "public/js/router.js",


why do we include both files ?

You're correct, shouldn't be including the first. Now updated.

stof · 2017-12-04T11:17:03Z

Resources/js/router.js

+     * Configures the current Router instance with the provided data.
+     * @param {Object} data
+     */
+    static setData(data) {


I suggest to also add a non-static method to simplify the usage in a CommonJS environment (and then, the static method kept for BC can call the non-static method internally to avoid duplication)

Good idea! now implemented.

stof · 2017-12-04T11:24:59Z

Resources/gulpfile.js

+const wrap = require('gulp-wrap');
+
+gulp.task('js', () => {
+    gulp.src('js/router.js')


should have a return statement, so that Gulp is notified about the result too (to know when it end, and whether it succeeded)

Done in a soon-to-be pushed commit.

stof · 2017-12-04T11:25:21Z

Resources/gulpfile.js

+});
+
+gulp.task('default', () => {
+    gulp.start(['js']);


missing return here too.

And btw, I don't think we need 2 different tasks.

Return added in a soon-to-be pushed commit.

Having two tasks isn't strictly necessary and I'm reasonably new to using Gulp so am happy to remove the second if you would prefer? My reading suggests that the 'standard' is to have a 'default' task which essentially acts as a 'controller' calling the other tasks which actually do the doing (so in a future version for example a 'css' task could be added)

stof · 2017-12-04T11:44:45Z

CONTRIBUTING.md


 ```bash
+$ cd Resources
 $ npm install google-closure-library


please it as a dev dependency, so that no extra manual installation is needed.

Good point, done in a soon-to-be pushed commit.

stof · 2017-12-04T11:45:23Z

CONTRIBUTING.md

 $ npm install google-closure-library
 ```

 Run the JS test suite with:

 ```bash
-$ phantomjs Resources/js/run_jsunit.js Resources/js/router_test.html
+$ phantomjs js/run_jsunit.js js/router_test.html


I suggest adding this command as the test script in package.json, so that the standard npm test command can be used.

Good call, done in a soon-to-be pushed commit. Documentation also updated.

weaverryan · 2017-12-16T00:52:46Z

Ping @matatirosolutions!

matatirosolutions · 2017-12-16T16:57:25Z

@weaverryan yeah - combination of somewhat overwhelmed by the feedback and not entirely sure how to move forward from this position, and swamped with clients who all suddenly want stuff done 'before the New Year'. Aim to get this sorted next week as I'm also working on an SF4 project which needs this merged!

weaverryan · 2017-12-17T04:13:26Z

Ping me here or on Symfony slack if you want to chat about any points. This is indeed a non/trivial task, but important!

tobias-93 · 2017-12-17T21:35:12Z

@matatirosolutions No worries, if you have any questions don't hesitate to ask. I've created a Gitter chatroom at https://gitter.im/FOSJsRoutingBundle/Lobby, in case you want to discuss something (and I encourage everyone here to join :)).

@stof

@stof

matatirosolutions · 2017-12-20T08:00:44Z

@weaverryan and @tobias-93 I've reverted the permissions changes on the JS files - these were the files brought in from the work which Bruno Sampaio did and I hadn't noticed that the permissions were different - thanks for spotting this!

@tobias-93 the reason that there are now two router.js files and not a symlink is that the version in the /js folder is written in ES6, and then cross-compiled (using gulp/babel) into the version in the /public folder which is ES5 compatible (and therefore backwards compatible with previous versions and older browsers) so we do now need the two separate files.

tobias-93

@matatirosolutions Thanks, good work! I noticed, however, that the Resources/ts/router.d.ts file still has changed permissions. Could you change that?
Furthermore: I'd like you to update the docs so it refers to the Resources/Public/js/router.min.js instead of Resources/Public/js/router.js as that changes the behaviour of the bundle.

tobias-93 · 2017-12-20T08:06:33Z

Resources/package.json

@@ -0,0 +1,47 @@
+{
+  "name": "fos-router",
+  "version": "1.6.0",


This should be updated to reflect the version number in the rest of the bundle

Hmmm... that was correct at the time of forking - the world of FOSJsRouting is moving quickly at present :-)

I've updated it to 2.2 - see if I can get the other feedback on this PR addressed before it's too late to be that version!

tobias-93 · 2017-12-20T08:07:40Z

Resources/js/router_test.html

-        <script language="javascript" type="text/javascript" src="../../node_modules/google-closure-library/closure/goog/base.js"></script>
-        <script language="javascript" type="text/javascript" src="router.js"></script>
+        <script language="javascript" type="text/javascript" src="../node_modules/google-closure-library/closure/goog/base.js"></script>
+        <script language="javascript" type="text/javascript" src="../public/js/router.js"></script>


Can the tests use the ES6 version? That way, you're always testing the non-compiled version, so compilation is not needed before you can run the tests.

Attempting to do that was where Bruno's original PR came unstuck (see #257) because in order to do that requires rewriting the test suite as well as the code. Understandably people didn't think that was a good idea, and that PR has been stalled for over a year.

After a discussions with @weaverryan and @stof at SymfonyCon, what I'm attempting to do here is to merge in the updated JS (to support webpack usage in SF4) without changing the test suite so that we can have a (high) degree of confidence that the new JS remains BC.

All right, then I will accept this. However, the test script should invoke the compile step before running the test, to prevent one from testing without compiling.

The docs should also be updated to reflect this (building before testing)

Sure thing. Running npm run test will now perform a build first. Docs updated to explain that this will happen.

tobias-93 · 2017-12-20T08:09:03Z

Resources/doc/usage.rst

@@ -1,7 +1,7 @@
 Usage
 =====

-Add these two lines in your layout:
+In applications not using WebPack add these two lines in your layout:


This is still not conform to the discussed capitalisation

…mentation

matatirosolutions · 2017-12-20T09:36:36Z

Thanks @willdurand, @weaverryan, @stof and @tobias-93 for your reviews.

I believe that I've now addressed all of the concerns and implemented all the changes requested. Please let me know if there's anything further required to merge this PR.

tobias-93

One last thing: I think we should adapt the Travis configuration to conform to the docs. Could you change the commands to npm install and npm test?

tobias-93 · 2017-12-20T11:37:34Z

CONTRIBUTING.md

 ```

+Because the current test suite runs agains the built javascript a build is automatically


Typo: agains instead of against

tobias-93 · 2017-12-20T12:28:03Z

I see that the PHP 5.3 test fails due to Node being too old. You could add a line to the travis config to skip the build on the old environment, as the tests themselves run fine.

…prevent unexpected dependencies in projects

matatirosolutions · 2017-12-21T17:09:45Z

In implementing this in an SF 4 project over the last two days I found that the use of the .babelrc file was resulting in the implementing project having to include the dependencies specified there in. To prevent that I've removed the .babelrc file and explicitly set the babel parameters in the gulp file.

steveWinter · 2018-01-07T10:59:38Z

@stof what needs to happen to move this forward - as far as I am aware I've addressed all of your concerns - are you able to take a look and confirm please?

steveWinter · 2018-02-07T07:54:02Z

What do I need to do to get this PR merged? As far as I'm aware I've addressed all concerns raised and it would be great if we could move forward with this!

weaverryan · 2018-02-07T19:49:56Z

Yea... I have the same question. @tobias-93: you approved this. Can you merge it?

tobias-93 · 2018-02-07T20:06:08Z

@weaverryan Yes, I approved this. I wanted to give @stof time to review this, because he had comments. However, I think he had his chance to do so. I'm now merging this.

weaverryan · 2018-02-07T20:55:43Z

Makes sense. And woohoo!!!! :)

weaverryan · 2018-02-11T14:18:25Z

@steveWinter I want to update the docs of using this bundle with Encore. What’s your setup look like? Are you always dumping the routes in dev and loading the JSON? Or something else?

steveWinter · 2018-02-13T13:13:00Z

@weaverryan I'm essentially doing things as per the update I made to the bundle docs.

And yes, I'm dumping the routes to json (though into /assets/js rather than the path in the docs) then in my 'layout.js' (as it was referred to in your SFCon workshop) I'm doing as per the link above.

On my build server I'm running the dump command before encore to ensure the built files have the latest set of routes.

weaverryan · 2018-02-13T20:35:23Z

Thanks @steveWinter :). I'm wondering if the bundle could/should (with some config) dump that JSON file automatically whenever the routing cache is rebuilt... that would avoid needing to re-dump it when you add new stuff. Not sure how possible that is...

steveWinter · 2018-02-14T09:46:18Z

@weaverryan I like the idea - if there are events dispatched as part of the route-cache building process, then it would be pretty simple but I know very little about the internals of Symfony routing, so have no idea how possible that might be.

I'd be happy to look at implementing this, if you can point me at the right person to talk to about the routing cache process.

stof · 2018-02-14T09:49:19Z

I don't think we can hook into the routing dumping easily.

weaverryan · 2018-02-14T14:33:20Z

Bah, I was afraid of that :). Maybe something to think about in core

ogizanagi · 2018-03-15T15:15:04Z

@weaverryan, @steveWinter : Actually, here is what I'm actually using with Webpack Encore to not have dumping routes each time on dev env (and also bundling everything in one file with Webpack):

`config/routes/dev/fos_js_routing.yaml`

# Only available in dev. On prod, use dumped routing.
fos_js_routing:
    resource: "@FOSJsRoutingBundle/Resources/config/routing/routing.xml"

`templates/base.html.twig`

[...]

{% block javascripts %}
    {# Only use the controller to get routes definitions in JS on dev mode. #}
    {# On prod, definitions are dumped and bundled by webpack. #}
    {% if 'dev' == app.environment %}
        <script>
          window.fosJsDevData = null;
          function storeFosJsDevData(data) {
            window.fosJsDevData = data;
          }
        </script>
        <script src="{{ path('fos_js_routing_js', { callback: 'storeFosJsDevData' }) }}"></script>
    {% endif %}
    <script src="{{ asset('build/js/app.js') }}"></script>
{% endblock %}

`assets/js/Routing.js`

import Routing from 'fos-routing';

// https://symfony.com/doc/master/bundles/FOSJsRoutingBundle/usage.html#generating-uris
if (typeof window.fosJsDevData !== 'undefined') {
  // On dev mode, we use the controller to get route definitions.
  // A Jsonp callback set the data in the "window.fosJsDevData" var,
  // so we can retrieve them here to configure the instance:
  Routing.setData(window.fosJsDevData);
}

export default Routing;

`webpack.config.js`

const Encore = require('@symfony/webpack-encore');
const webpack = require('webpack');

// FOS Js Router files
const FOSJsRouting = [];
// Use dumped routes on production:
if (Encore.isProduction()) {
  // this is the path where I dump routes when building the app on production
  FOSJsRouting.push('./public/bundles/fosjsrouting/js/fos_js_routes.js');
}

Encore
     // [...]
    .addEntry('js/app', [
      ...FOSJsRouting,
      './assets/js/app.js',
    ])

Finally, I'm using the following to dump on production:

bin/console fos:js-routing:dump --target=public/bundles/fosjsrouting/js/fos_js_routes.js --callback="import Routing from 'fos-routing'; Routing.setData"

... for which the callable argument is more or less a hack since this PR as the window.fos var is not exposed anymore when bundling the router using webpack... (default callback is fos.Router.setData)

steveWinter added 4 commits November 19, 2017 13:06

First commit integrating refactored JS code from Bruno Sampaio to rem…

8ee9b06

…ove reliance on Google Closure and support WebPack integration

Resolving issues with failing tests

f2d79b0

Moving location of npm install call to match updated router_test.html

8ad9864

Updating documentation with WebPack information

b2544fc

willdurand reviewed Dec 4, 2017

View reviewed changes

stof requested changes Dec 4, 2017

View reviewed changes

steveWinter added 4 commits December 19, 2017 18:22

Updating capitalisation of webpack as requested.

1ca8e9d

Adjusting spacing for note block

9163d0f

Updates to gulpfile, package and documentation addressing comments from

0c05b6c

@stof

Setting correct permissions on js files imported from Bruon's work

d84ebcf

tobias-93 requested changes Dec 20, 2017

View reviewed changes

steveWinter added 2 commits December 20, 2017 10:30

Correcting permissions, capitalisation, and version number

12f044f

Adding non-static method for route setting to simplify commonJS imple…

8875861

…mentation

Run a build prior to the JS test suite. Updated documentation.

e561b08

tobias-93 requested changes Dec 20, 2017

View reviewed changes

Switching Travis to use the same install and test commands as documented

f21195d

steveWinter added 2 commits December 20, 2017 14:31

Updated gulpfile in an attempt to support PHP 5.3 on Travis

ca00f19

Updated gulpfile removing another choke point for Travis on PHP 5.3

d2688e3

tobias-93 approved these changes Dec 20, 2017

View reviewed changes

Setting babel parameters in gulpfile to allow removal of .babelrc to …

e7fab29

…prevent unexpected dependencies in projects

tobias-93 merged commit 6aa4356 into FriendsOfSymfony:master Feb 7, 2018

tobias-93 mentioned this pull request Feb 7, 2018

JS refactor - remove dependency on Google Closure Library #257

Closed

stof mentioned this pull request May 22, 2018

Typescript declaration is outdated #323

Closed

		```

		Because the current test suite runs agains the built javascript a build is automatically

JavaScript refactoring #303

JavaScript refactoring #303

Conversation

matatirosolutions commented Nov 19, 2017

weaverryan commented Dec 3, 2017

tobias-93 commented Dec 4, 2017

willdurand 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

willdurand Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand commented Dec 4, 2017

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

weaverryan commented Dec 16, 2017

matatirosolutions commented Dec 16, 2017

weaverryan commented Dec 17, 2017

tobias-93 commented Dec 17, 2017

matatirosolutions commented Dec 20, 2017

tobias-93 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

matatirosolutions commented Dec 20, 2017

tobias-93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobias-93 commented Dec 20, 2017

matatirosolutions commented Dec 21, 2017

steveWinter commented Jan 7, 2018

steveWinter commented Feb 7, 2018

weaverryan commented Feb 7, 2018

tobias-93 commented Feb 7, 2018

weaverryan commented Feb 7, 2018

weaverryan commented Feb 11, 2018

steveWinter commented Feb 13, 2018

weaverryan commented Feb 13, 2018

steveWinter commented Feb 14, 2018

stof commented Feb 14, 2018

weaverryan commented Feb 14, 2018

ogizanagi commented Mar 15, 2018 • edited Loading

config/routes/dev/fos_js_routing.yaml

templates/base.html.twig

assets/js/Routing.js

webpack.config.js

willdurand Dec 4, 2017 •

edited

Loading

ogizanagi commented Mar 15, 2018 •

edited

Loading

`config/routes/dev/fos_js_routing.yaml`

`templates/base.html.twig`

`assets/js/Routing.js`

`webpack.config.js`