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

Source maps do not have to be on their own line #133

Conversation

pond
Copy link
Contributor

@pond pond commented Mar 7, 2023

(NB: This PR builds on #132. I can't see a way to do a pull request on top of the prior pull request, so the diff on this one will make more sense once #132 is completed. I had to build on it since the source map removal logic of #132 is now extended further.)

While some older resources claim that source maps must be on their own line at the end of a file and while Propshaft's Regexp for this asserts such, this is not the case. Consider:

sass ./app/assets/stylesheets/application.scss ./app/assets/builds/application.css --no-source-map --style compressed --load-path=node_modules

...in package.json. This results in application.css including a source map comment in /* ... */ form at the end of the last line of minified CSS:

<very-long-line>...vertical-align:middle}/*# sourceMappingURL=application.css.map */

...noting that a terminating line feed is also present afterwards. Since this is a CSS comment appearing at the end of a line it is syntactically valid and browsers such as Chrome do read the source map. Under e.g. Propshaft 0.7.0, CSS source maps promptly break because it doesn't consider the not-on-own-line map comment to be valid.

  • It seems that so long as the comment is the last thing in the file, whether on its own line or not, then it is processed
  • Source map comments in the middle of the file are, as before, not processed
  • The regexp is therefore updated to check for end-of-stream (\Z) rather than start-of-line (^)
  • This means it might optionally capture a closing */ in comments of that form, so the internal private methods now need to be given both an opening and closing string for the comment captures
  • Test coverage for JS with //# and CSS with /* ... */ style comments included

@pond
Copy link
Contributor Author

pond commented Mar 21, 2023

Bump on this.

@brenogazzola
Copy link
Collaborator

Do you need to make any changes to this now that #132 is merged?

…e/sourcemaps-do-not-have-to-be-on-their-own-line
@pond
Copy link
Contributor Author

pond commented May 8, 2023

@brenogazzola Thanks for the reminder - updated to resolve conflicts after merge of #132 and now ready for review.

@brenogazzola
Copy link
Collaborator

@pond I apologize for not merging this PR for so long. I was going to merge it now, but it seems there are conflicts again. Could you fix them, and I'll immediately merge this?

…e/sourcemaps-do-not-have-to-be-on-their-own-line
@pond
Copy link
Contributor Author

pond commented Sep 16, 2023

@brenogazzola I too have been extraordinarily busy and apologies in turn for taking so long to fix the conflicts - they were a little fiddly. It's done now and CI is green. Hopefully a merge can happen before conflicts re-emerge! :-D

@pond
Copy link
Contributor Author

pond commented Sep 20, 2023

@brenogazzola (bump on this, thanks...)

@brenogazzola brenogazzola merged commit 96ddc84 into rails:main Sep 20, 2023
4 checks passed
@brenogazzola
Copy link
Collaborator

@pond Thanks!

@pond
Copy link
Contributor Author

pond commented Sep 20, 2023

Ah! Great. Legend. Thanks :-)

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.

2 participants