Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Add source maps for generated code #7

Merged
merged 4 commits into from
May 12, 2017
Merged

Add source maps for generated code #7

merged 4 commits into from
May 12, 2017

Conversation

kitsonk
Copy link
Member

@kitsonk kitsonk commented May 5, 2017

Feature

This PR provides support for source maps in generated code. It also "names" the scripts in a way that is recognised by some debuggers that make debugging dynamically generated applications from the Runner easier to debug. In Chrome DevTools, breakpoints can be set on the generated runner applications.

This also includes source maps for CSS as well, which Chrome DevTools will automatically recognise.

The dynamically generated JavaScript will be located under the runner iframe:

_dojo_web-editor

For some reason the original TypeScript is not visible at this point, but if you wanted to set a breakpoint on the generated code, you would open the emitted JavaScript:

_dojo_web-editor

And when you click on the gutter to set the breakpoint, Chrome will realise it has a source map and open up the original TypeScript file:

_dojo_web-editor

After this point, debugging the application should work, remembering the breakpoints.

@codecov
Copy link

codecov bot commented May 5, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@095ecb8). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #7   +/-   ##
=========================================
  Coverage          ?   57.63%           
=========================================
  Files             ?       15           
  Lines             ?      491           
  Branches          ?       87           
=========================================
  Hits              ?      283           
  Misses            ?      208           
  Partials          ?        0
Impacted Files Coverage Δ
src/support/base64.ts 100% <100%> (ø)
src/support/css.ts 100% <100%> (ø)
src/Runner.ts 100% <100%> (ø)
src/support/sourceMap.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 095ecb8...a3b2150. Read the comment docs.

dylans
dylans previously requested changes May 5, 2017
@@ -2,15 +2,17 @@ import { ProjectFileType } from '@dojo/cli-export-project/interfaces/project.jso
import Evented from '@dojo/core/Evented';
import { createHandle } from '@dojo/core/lang';
import project from './project';
import * as base64 from './support/base64';
Copy link
Member

Choose a reason for hiding this comment

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

Does this belong as a support helper, or something in core, or revisit doing something with crypto? Not a blocking comment, just something we should open an issue for elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly... but I wasn't sure where.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's just open an issue in meta and decide that later.

Copy link
Member Author

Choose a reason for hiding this comment

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

A related defect is essentially here: dojo/core#299 which I should port over and fix and then refactor here off of core.

src/Runner.ts Outdated
for (const mid in modules) {
modulesText += `\t'${mid}': function () {\n${modules[mid]}\n},\n`;
/* inject each source module as it's own <script> block */
Copy link
Member

Choose a reason for hiding this comment

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

it's -> its

request.setDefaultProvider(getProvider(require));
require([ 'src/main' ], function () { });
});
<script>require.config({
Copy link
Member

Choose a reason for hiding this comment

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

Is this section intentionally unindented?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because it the view in the debugger much cleaner and more readable.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, no problem.

@@ -176,7 +204,7 @@ registerSuite({
await runner.run();
const doc = getDocFromString(getDocumentStrings(iframe)[0]);
const scripts = doc.querySelectorAll('script');
assert.lengthOf(scripts, 4, 'should have four script nodes');
assert.lengthOf(scripts, 5, 'should have four script nodes');
Copy link
Member

Choose a reason for hiding this comment

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

should have four script nodes -> should have five script nodes

@dylans dylans added this to the 2017.05 milestone May 5, 2017
@kitsonk kitsonk force-pushed the feature-source-map branch from c1ddf22 to fa8fa7c Compare May 12, 2017 15:04
@kitsonk kitsonk dismissed dylans’s stale review May 12, 2017 15:05

comments addressed

@kitsonk kitsonk merged commit b5f7a10 into master May 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants