-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
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. |
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... |
Honestly, I don't see what it brings us, but I can see that merging things from older branches will be more challenging. |
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 From a developer pov it may even enhance DX, as pure JS files has benefits (IDE-wise, linting, etc.). |
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. |
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. |
Fair enough. It's not worth it then. |
POC to separate the WDT javascript assets from twig templates.
Pros
I propose to go with a
toolbar.js
andprofiler.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.