Skip to content

[WebProfilerBundle][POC] Separate JS assets #21050

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 1 commit into from
Closed

[WebProfilerBundle][POC] Separate JS assets #21050

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Dec 25, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

POC to separate the WDT javascript assets from twig templates.

Pros

  • Separation of concerns
  • Better maintenance / features can be separated
  • Allows compiling (currently toolbar loads toggle/tabs utils and profiler loads ajax utils)
  • Allows browser caching
  • Allows proper minifying

I propose to go with a toolbar.js and profiler.js, built from individual files (base.js, ajax.js, toggles.js, etc.)

If accepted, we can also try a lower branch first. But it should go at once imo.

@fabpot
Copy link
Member

fabpot commented Dec 26, 2016

Keep in mind that the profiler is a dev tool. So, "proper minifying" is a non-goal and won't happen. The same goes for "Load only what's needed". So, I can be ok to extract the JS to another file to ease maintenance, but I'm 👎 for everything else.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 26, 2016

It's a debug tool. That said.. your're right; i have no intentions to over-engineer this or revise the whole thing.

The list in the ticket description is a list of pros, not necessarily goals itself 👍

The true goal is to separate JS from twig (and probably CSS as well), i dont see the need for it. But maybe im overlooking some implementation details here?

Next, the PR loads JS from an external file, would that be ok? I guess it's an implementation detail...

@fabpot
Copy link
Member

fabpot commented Dec 26, 2016

Honestly, I don't see what it brings us, but I can see that merging things from older branches will be more challenging.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 26, 2016

Imo. clean code, easier to review and maintain. But it really depends how much stuff is added in the long run.

For example, only twig has to do is something like Sfjs.call('{{ token }}', '{{ path () }}'); to bridge some things.

From a developer pov it may even enhance DX, as pure JS files has benefits (IDE-wise, linting, etc.).

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 26, 2016

To clarify; moving JS away from twig (at this point) is easy, little effort. Now compared to the potential gain, it could be worth it.

@fabpot
Copy link
Member

fabpot commented Dec 26, 2016

This code does not change that often and we don't plan to add more JS in the long run. DX, IDE, linting, ... does not make sense for internal code IMHO.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 26, 2016

Fair enough. It's not worth it then.

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.

3 participants