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

Fix source maps caching key #367

Merged
merged 1 commit into from Aug 25, 2016
Merged

Conversation

Strech
Copy link
Contributor

@Strech Strech commented Aug 18, 2016

Closes #344

Add processed source file dependencies to the source map.
This asset call @env['application.js.map'] will add compiled application.js dependencies with all derectives to the application.js.map asset

Closes rails#344

Add processed source file dependencies to the source map.
This asset call `@env['application.js.map']` will add compiled
`application.js` dependencies with all derectives to the
`application.js.map` asset
@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Strech
Copy link
Contributor Author

Strech commented Aug 18, 2016

A bit about my investigation:

I was trying to make it in other way - through normal pipeline process, but I've failed. When we are requesting application.js.map and if we have application.coffee source file, the mime type system will recognise it as type = "application/js-sourcemap+json" and file_type = text/coffeescript.

And the pipeline will looks like that: [SourceMapProcessor, DefaultSourceMap, DirectiveProcessor, CoffeeScriptProcessor]

So this mean that if I have require directive inside my application.coffee file, it will fail to find this require statement file, because the initial type which will be passed as accept option to resolve method in DirectiveProcessor will be application/js-sourcemap+json. And basicaly this is the main issue - type mismatch.

We should have application/javascript type for all the processors, exept SourceMapProcessor. But I can't find a simple solution for that. So I decide that it's better to keep all the ugly code in one place, to be sure that it will not speding out everywhere and in case of a better folution we can easily wipe it out.

Maybe if you have any suggestions to implement it better way, I would be glad to hear.
Thanks!

@Strech
Copy link
Contributor Author

Strech commented Aug 24, 2016

@rafaelfranca If you have a time, can you 👀 on this PR?

@schneems schneems merged commit efe0564 into rails:master Aug 25, 2016
@schneems
Copy link
Member

I was able to repro with the linked example app, and I confirm your patch fixes it.

A source map should depend on the same things it's parent's dependencies. Thanks a ton for your help and this patch.

@Strech Strech deleted the fix/source-map-sources branch August 25, 2016 18:17
@Strech
Copy link
Contributor Author

Strech commented Aug 25, 2016

@schneems No problem, I want to help your team more 👍

@TylerHorth TylerHorth mentioned this pull request Oct 13, 2016
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