8000 [AssetMapper] Disable profiler when the "dev server" respond by smnandre · Pull Request #52100 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Disable profiler when the "dev server" respond #52100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 17, 2023
Merged

[AssetMapper] Disable profiler when the "dev server" respond #52100

merged 1 commit into from
Oct 17, 2023

Conversation

smnandre
Copy link
Member
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT

In debug mode, the "dev server" send assets as if they were compiled in the public directory.

To improve the DX and not "pollute" too much the profiler (see capture below), this PR does two things :

  • disable the profiler for those requests
  • stop the request & response event propagation, to speed things a bit
Capture d’écran 2023-10-17 à 10 57 44

@carsonbot carsonbot added this to the 6.4 milestone Oct 17, 2023
nicolas-grekas
nicolas-grekas approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's one thing I'm not comfortable with in this class, it's string $publicPrefix = '/assets/'.
This works only when the app is hosted at the root of the domain, and we know that's not always the case, and that we can deal with this in other components.
That's unrelated to this PR but still :)

@smnandre
Copy link
Member Author
smnandre commented Oct 17, 2023 •
edited
Loading
8000

There's one thing I'm not comfortable with in this class, it's string $publicPrefix = '/assets/'.

I agree, but how could we mimic an external CDN on localhost ? :| (missread, sorry)

How would you handle that ?

@nicolas-grekas
Copy link
Member

The routing component is the one that can deal with this. IIRC, it uses RequestContext to do so.
Instead of hardcoding the prefix, maybe we could let the routing component match the request and act on that?
That'd require ppl to define a route in their apps (thanks to a recipe that should be easy). Then, this listener would act when the request has an attribute (_controller?) that matches some convention to trigger it.
WDYT? /cc @weaverryan

@smnandre
Copy link
Member Author

Instead of hardcoding the prefix

It's just a default value... the instance from the container uses the "asset_mapper.public_prefix"

fabpot
fabpot approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

We might consider changing the Response header name? @weaverryan?

@weaverryan
Copy link
Member

The routing component is the one that can deal with this. IIRC, it uses RequestContext to do so.
Instead of hardcoding the prefix, maybe we could let the routing component match the request and act on that?

I'm not sure there's a problem - the logic for checking to see if this controller should be used is:

$pathInfo = $event->getRequest()->getPathInfo();
if (!str_starts_with($pathInfo, $this->publicPrefix)) {
    return;
}

So the pathInfo removes the subdirectory for the comparison. I just tested with a subdirectory, and the paths all seem fine - but let me know if you still see some risk.

@nicolas-grekas
Copy link
Member

Thanks for checking 🚀

@nicolas-grekas nicolas-grekas force-pushed the dx/asset-mapper-server-disable-profiler branch from 26bd310 to e1174ac Compare October 17, 2023 13:12
@nicolas-grekas
Copy link
Member

Thank you @smnandre.

@nicolas-grekas nicolas-grekas merged commit 9233ea8 into symfony:6.4 Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
AssetMapper Feature Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
5 participants
@smnandre @nicolas-grekas @weaverryan @fabpot @carsonbot
0