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

Better error reporting and clean up of libdeno namespace #488

Merged
merged 7 commits into from Aug 9, 2018

Conversation

3 participants
@ry
Collaborator

ry commented Aug 9, 2018

This is mostly the changes that were in #452 but it was getting to be quite complicated, so I'm doing this in a separate PR.

@ry ry requested a review from piscisaureus Aug 9, 2018

@ry ry referenced this pull request Aug 9, 2018

Merged

readFileSync #452

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 9, 2018

Collaborator
Collaborator

ry commented Aug 9, 2018

let r = Url::from_file_path(module_specifier);
// TODO(ry) Properly handle error.
if r.is_err() {
error!("Url::from_file_path error {}", module_specifier);

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I'm not sure what's the practical difference with the situation prior to this patch.
But it's not worse either. 🤷‍♂️

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I'm not sure what's the practical difference with the situation prior to this patch.
But it's not worse either. 🤷‍♂️

This comment has been minimized.

@ry

ry Aug 9, 2018

Collaborator

It sometimes would fail on windows due to not having "C:/" ... and it wouldn't print anything.

@ry

ry Aug 9, 2018

Collaborator

It sometimes would fail on windows due to not having "C:/" ... and it wouldn't print anything.

Show outdated Hide outdated tools/format.py Outdated
@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I don't see any "clean up of libdeno namespace". This PR complete?

Collaborator

piscisaureus commented Aug 9, 2018

I don't see any "clean up of libdeno namespace". This PR complete?

Show outdated Hide outdated js/globals.ts Outdated
Show outdated Hide outdated js/globals.ts Outdated
HandleException(context, try_catch.Exception());
return;
}
auto source_map_obj = script.ToLocalChecked()->Run(context);

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I'm a little worried that this causes both the object and the string to be embedded in the binary. Did you check?

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I'm a little worried that this causes both the object and the string to be embedded in the binary. Did you check?

This comment has been minimized.

@ry

ry Aug 9, 2018

Collaborator

yes, I checked the binary size remains the same

@ry

ry Aug 9, 2018

Collaborator

yes, I checked the binary size remains the same

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

Ah, I see that before this patch the source map JSON was decoded in a similar way, so it wouldn't make a difference.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

Ah, I see that before this patch the source map JSON was decoded in a similar way, so it wouldn't make a difference.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

Also reviewed the "Organize libdeno functions." commit now. LGTM

Collaborator

piscisaureus commented Aug 9, 2018

Also reviewed the "Organize libdeno functions." commit now. LGTM

@ry ry merged commit 038c5f0 into master Aug 9, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ry ry deleted the error_reporting branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment