Skip to content
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

Handle sourceMappingURL already prefixed with asset path #170

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

tagCincy
Copy link
Contributor

@tagCincy tagCincy commented Dec 8, 2023

When using a build system, specifically esbuild and referencing assets such as images, you must set a publicPath configuration. This allows assets to be fetched for the appropriate endpoint (eg. image.png --> assets/image.png) when rendering a page. In addition to images being called from url(), setting the publicPath also sets the sourceMappingURL comment to the prefixed path. Because of this added prefix, Propshaft removes the sourceMappingURL comment from the minified JS asset as the path cannot be correctly resolved.

This change will remove the prefix if the source_mapping_url prior to rewrite of the comment.

@tagCincy tagCincy marked this pull request as ready for review December 8, 2023 05:05
@brenogazzola
Copy link
Collaborator

This seems to be addressing the same problem as #150. Or is it something else?

@brenogazzola
Copy link
Collaborator

If it is, I'm more inclined with going with this solution, instead of allowing the regex to swallow everything that is not the asset name in a non capturing group.

@tagCincy
Copy link
Contributor Author

Yes it is

@tagCincy tagCincy closed this Dec 11, 2023
@brenogazzola
Copy link
Collaborator

Why did you close it? This seems like the better solution compared to #150

@tagCincy tagCincy reopened this Dec 11, 2023
@tagCincy
Copy link
Contributor Author

Reopened. I found a monkey patching solution to unblock me.

in production environments the url_prefix may be proceeded by a cdn host, ensure that it is part of the capture group
this change will require anything prior to the url_prefix to end in a forward slash
test case
@brenogazzola brenogazzola merged commit a043d16 into rails:main Jan 22, 2024
4 checks passed
@brenogazzola
Copy link
Collaborator

Thanks!

@lvangool
Copy link

We're caught by this too - any ideas when the next cut of propshaft will be published?

@brenogazzola
Copy link
Collaborator

brenogazzola commented Jan 24, 2024

I'll ask, but for now you can simply change your Gemfile to point to the correct commit in the main (which is what the new gem would use)

gem "propshaft", github: "rails/propshaft", ref: "8fb8f7c9cb63c16b381dad47b69a36fd67837985"

Or if you prefer, point to main (which is what I do):

gem "propshaft", github: "rails/propshaft", branch: "main"

@lvangool
Copy link

@brenogazzola thank you! We're not keen to run a version that isn't also being run by thousands of other people; would be great to have an indication of when an official cut is scheduled 🙏🏻

@lvangool
Copy link

@brenogazzola any chance you managed to get an approximate release date?

@pfeiffer
Copy link

Would love to see this included in a release!

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

Successfully merging this pull request may close these issues.

None yet

4 participants