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.

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

source mapping tweaks #106

Closed
wants to merge 12 commits into from
Closed

source mapping tweaks #106

wants to merge 12 commits into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jun 10, 2019 •
edited

JIRA: https://jira.appcelerator.org/browse/TIMOB-27098

Description
Modifies our behavior with source maps.

The net result of all my testing is this:

  • iOS blows up if the source map's "sources: ["filename.js"]" name is not the base name of the JS file. Presumably because it has to match the script name it shows for the generated/in-app file (which is just the base name). If they don't match but it's able to load the source map, it'll show a folder child for the script, but it will be empty and the source mapping is busted. There's probably some more work to do digging here. We report full file URIs with absolute paths to the JS engine under the hood for iOS. Maybe using relative (to app root) paths would fix that? We report URI paths that looks absolute but are relative to the app's Resources folder on Android.
  • Our require implementation for iOS wraps the original content injecting a ; right after it, which is why it couldn't load the inlined source maps as-is. I worked around this by writing the inlined source maps ourselves with extra newlines around it.
  • Android is happy to use the script's reported //# sourceURL=file:///path/to/source.js value even if the source map has just the base name of the file (as iOS needs). It then organizes the sources!

I tested this on Android Emulator and iOS Simulator, but not devices

Screen Shot 2019-06-11 at 3 53 10 PM
Screen Shot 2019-06-11 at 3 51 37 PM

@sgtcoolguy sgtcoolguy mentioned this pull request Jun 10, 2019
Closed
@build
Copy link

build commented Jun 10, 2019 •
edited

Messages
📖

✅ All tests are passing
Nice one! All 26 tests are passing.
📖 🎉 - congrats on your new release

Generated by 🚫 dangerJS against 28f7079

@sgtcoolguy
Copy link
Contributor Author

One thing to note with inlined source maps is that we did not specify an absolute path to the input source file, so the generated map just reports a path like "sources": ["app.js"] which isn't right because we have no base path to resolve it from. I can hack around that issue in Studio, but I suspect this is part of why Web Inspector doesn't like our inlined source maps (though fixing the path or setting a sourceRoot doesn't seem to help, it's broken in a different way)

@sgtcoolguy
Copy link
Contributor Author

So I'm still trying to make iOS/Web Inspector happy here. The notable thing is that pointing to an external source map does not seem to work with iOS - maybe it can't handle file: URIs or is building up the path to the .map file inside the app incorrectly?

The existing approach of using inlined source maps fails due entirely to the way iOS blindly does it's require implementation which injects a ; after the input source file - causing it to get appended to the sourceMappingURL value (breaking the base64 encoded contents)! So if we inject a newline to the generated code, using inline source maps does work for iOS.

So far, Android is happiest with an external .map file and pointing at it with an absolute file URI.
iOS is happiest with an inlined source map, so long as we work around the ugly ; injection.

I don't have an Android device handy to know if using external .map files would work in that case or we'd have to move to inlined source maps...

@sgtcoolguy
Copy link
Contributor Author

Note that with the latest PR here, we no longer need tidev/titanium-sdk#10954

@sgtcoolguy sgtcoolguy mentioned this pull request Jun 11, 2019
Merged
@sgtcoolguy
Copy link
Contributor Author

relates to tidev/titanium-sdk#10951 and appcelerator/titanium_studio#1134

@ewanharris
Copy link
Contributor

@sgtcoolguy, is tidev/alloy#893 relevant here too? If so we'll need to pull it into Aloy 1.14.0/CLI 7.1.0

@sgtcoolguy
Copy link
Contributor Author

@ewanharris They are related, but there's so many "moving parts" that I wanted to nail down this PR and the Studio one first and get those working, then we can see if we can fix the Alloy app/ -> Resources/ -> build/ mapping chain.
I assume we'll need to tweak node-titanium-sdk to fix the //# sourceURL= file path I'm injecting here to point at the original alloy app/ file - and likely to tweak it to take in the input source map and see if it cam properly combine it with the transpiled one (and see if we need to tweak the "sources" paths used from Alloy so it doesn't end up breaking the chain)

@sgtcoolguy
feat(jsanalyze): support consuming input inline data: source maps
515fbd4
@sgtcoolguy
test(jsanalyze): add tests for processing input JS file with inline s…
cdd1467 
…ource map
@sgtcoolguy
chore(release): bump version to 3.2.0
aa15e12
@sgtcoolguy
fix(jsanalyse): set sourceRoot in final sourcemap, delete sourcesCont…
535e9e1 
…ent on Android, do not inject sourceURL
@sgtcoolguy
test(jsanalyze): update test assumptions
855c73b
@sgtcoolguy
fix(jsanalyze): fix handling input source maps
28f7079 
On iOS, if source basename matches generated file basename, just report basename as source, not a path
or else Safari/Web Inspector will not show the file. This can lead to original source files
not being shown at all if their basename matches that of another source.
It also means the source entries in iOS source maps may be "wrong" in that they strip the full path.
Consumers trying to resolve back to original filepaths will need to have some knowledge of how we copy files.
The only way to get around this is to try and avoid matching basenames between multiple sources and between source
and generated filename.
@sgtcoolguy
Copy link
Contributor Author

OK, latest update:

  • Don't use //# sourceMap= comments. The JS engines will then report that as the generated file URL in place of the real one and we lose that info (depending on platform/OS version they may handle it correctly)
  • Alloy had some off-by-one errors in generated source maps once we hit the template marker points, pushed a fix to the PR.
  • Alloy reported a relative path for the generated file in the source map - but sourceRoot only applies to sources, not file in the spec - so I changed that to report full file path in the PR.
  • I had to make some tweaks here to properly consume and merge the input and output source maps. And it did require getting the sources/sourceRoot correct on Alloy's source maps. (And I think requires bed of file paths, not file:// URIs)
  • I made Alloy spit out inlined source maps for local testing and used http://sokra.github.io/source-map-visualization/#custom to confirm both the alloy maps and the combined maps. Once it was correct, I reverted to external .map files for the Alloy PR.
  • There's a bug in iOS/Safari/Web Inspector that I could not find how to get around:
    • If there's a source map and one of the sources has the same base name as the generated file, but has additional path components, Safari's debugger will show the path components as a folder in the Scripts tree but will not show the source file under it! My "fix" is to strip the additional path components and just report the source as the base name. That fix works visually, but introduces two side-effects:
      • We lose the real file path to the source, so something like Studio has to know how to back-track from generated file to source file without relying on the source map.
      • If multiple sources have the same base name only one "wins" and get's shown. The other basically gets dropped. In my testing it happened to drop the alloy template source, which is OK as we'd prefer to keep the user's controller/widget/whatever code over the template it gets injected into.

I do not know why iOS/Safari acts this way. We can help avoid part of the side-effect it if we report a different base name for the template source (maybe even just dummy up a name like template.js). Note that a related thing here is that for Android it's debugger can mostly look up source contents from the sources entries, so I wipe the sourcesContent entry in the source map to save space - which makes it show empty contents for the template source entries; and if we pointed at the actual template file it could then open/show it. (Although there's an issue there where Alloy's source map refers to the lines in the partially filled template, not the original template file)
As for why it insists the generated file base name be what is reported by the sources entry if they share a base name - no idea. Maybe tweaking SDK internals to report different URLs for scripts when we load them would help fix it? iOS and Android differ internally in that way - Android reports URLs like "/app.js", while iOS reports full file URIs like: "file:///Users/cwilliams/..../defaultClassic.app/app.js"

@sgtcoolguy
Copy link
Contributor Author

Changing iOS to report urls like "/app.js" didn't help any - so long as the generated file's base name matches the source file's base name, it shows a folder for the source path but no child file with the source file. Safari is just... not right in the head.

@sgtcoolguy
Copy link
Contributor Author

sgtcoolguy commented Jun 19, 2019 •
edited

Manually rebased and merged to master

@sgtcoolguy sgtcoolguy closed this Jun 19, 2019
@ewanharris ewanharris mentioned this pull request Jul 2, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewanharris ewanharris Awaiting requested review from ewanharris

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgtcoolguy @build @ewanharris