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 map support #429

Merged
merged 2 commits into from
Aug 2, 2018
Merged

Source map support #429

merged 2 commits into from
Aug 2, 2018

Conversation

ry
Copy link
Member

@ry ry commented Jul 30, 2018

No description provided.

@ry ry force-pushed the source_map_with_asm branch 2 times, most recently from dc3f52c to 2f50682 Compare July 30, 2018 20:31
@ry ry mentioned this pull request Jul 31, 2018
Closed
kitsonk
kitsonk reviewed Jul 31, 2018
View reviewed changes
js/runtime.ts
util.assert(Number(rawSourceMap.version) === 3);
mainSourceMap = rawSourceMap;
}
window["setMainSourceMap"] = setMainSourceMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to reflect this in lib.deno.d.ts when that has landed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - done. (Although it's global right now. Will fix later)

src/binding.cc Outdated
skip_onerror = true;
{
auto source = deno::v8_str(js_source.c_str());
CHECK(global->Set(context, deno::v8_str("mainSource"), source).FromJust());
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to reflect this in lib.deno.d.ts when it lands into master.

@piscisaureus
Copy link
Member

Didn't review the code yet - need to catch up on sleep before I can do it with a whole mind. I did try it out:

  • 2f50682 sourcemaps -> fails with the node.d.ts not picked up by typescript issue you know about.
  • 8dab9f7 use_asm -> GN error 
    DENO_BUILD_ARGS: ['is_official_build=true']
C:\Users\BertBelder\d\deno\third_party\depot_tools\gn gen C:\Users\BertBelder\d\deno\out\release
ERROR at //build_extra/deno.gni:39:19: Undefined identifier
      rebase_path(snapshot_out_cc, root_build_dir),
                  ^--------------
See //BUILD.gn:268:1: whence it was called.
create_snapshot("deno") {
^------------------------
  • d7365e4 exception handling -> build/test passes.

I noticed that deno and deno_ns tests run a lot faster now, even in debug mode.
Presumably due to the code caching patch? Either way, it's awesome!

@ry
Copy link
Member Author

ry commented Aug 1, 2018

@piscisaureus GN error fixed and source map commit removed in https://github.com/ry/deno/pull/435

I will leave this PR open for just the source map commit (which I will rebase on #435 after it lands)

@ry ry force-pushed the source_map_with_asm branch 3 times, most recently from 5badb98 to 7556f14 Compare August 1, 2018 16:24
@ry ry changed the title Source map with asm Source map support Aug 1, 2018
@ry
Copy link
Member Author

ry commented Aug 1, 2018

@piscisaureus I've added a triple slash directive which potentially could solve your issue.... Does it work?

@ry ry mentioned this pull request Aug 1, 2018
Merged
piscisaureus
piscisaureus approved these changes Aug 2, 2018
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

It's unfortunate that the source map is so big. I think it should be fixed with some priority - maybe open an issue so another contributor can look at it?

LGTM

src/snapshot_creator.cc
@@ -45,18 +47,25 @@ v8::StartupData MakeSnapshot(const char* js_filename, const char* js_source) {
int main(int argc, char** argv) {
const char* snapshot_out_bin = argv[1];
const char* js_fn = argv[2];
const char* source_map_fn = argv[3]; // Optional.
Copy link
Member

Choose a reason for hiding this comment

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

I still find _fn a confusing abbreviation for filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just following the convention here. I'll avoid it in the future.

js/v8_source_maps.ts
// dependency on the Node types.
// TODO(ry) Ideally this triple slash directive should be removed as we only
// need CallSite and Error.prepareStackTrace but nothing else.
/// <reference types="node" />
Copy link
Member

Choose a reason for hiding this comment

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

We could also just copy those definitions into our own repo.
After all they're not really node specific - but from v8.

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 - into lib.deno.d.ts - I think that's probably the ideal solution. I'll leave this for now since it's easy.

js/lib.deno.d.ts
mainSource: string; // TODO(ry) This shouldn't be global.
// TODO(ry) These shouldn't be global.
mainSource: string;
setMainSourceMap(sm: string): void;
Copy link
Member

Choose a reason for hiding this comment

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

string | RawSourcemap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually fixing this caused some unknown error. I'm backing out of that and landing as originally approved. Will fix in follow up.

js/lib.deno.d.ts
declare const window: Window;

// TODO(ry) These shouldn't be global.
declare let mainSource: string;
Copy link
Member

Choose a reason for hiding this comment

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

This seems duplicative with the Window interface above. Do we really need both?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we don't need either - but I'm not sure the right solution now. I will fix it later.

ry added 2 commits August 2, 2018 12:51
@ry
Source map support.
6a440dc 
This change increases size:
out/debug/obj/libdeno/from_snapshot.o  19M -> 34M
out/release/deno                       32M -> 47M
@ry
Try adding <reference types="node" />
81f96f6
@ry ry force-pushed the source_map_with_asm branch 2 times, most recently from dfd3206 to 81f96f6 Compare August 2, 2018 17:01
@ry ry merged commit c7c6203 into master Aug 2, 2018
@ry ry deleted the source_map_with_asm branch August 2, 2018 17:13
piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019
@Nautigsam @piscisaureus
http: send an empty response body if none is provided (denoland#429)
e00e3fe 
Fixes: denoland#402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kitsonk kitsonk kitsonk left review comments

@piscisaureus piscisaureus piscisaureus approved these changes

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
@ry @piscisaureus @kitsonk