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

Onerror Callback Support for Errors Fetching External Import Map #2324

Merged
merged 4 commits into from Apr 28, 2021

Conversation

kykwak
Copy link
Contributor

@kykwak kykwak commented Apr 28, 2021

Hi,

These changes are for being able to execute custom error handling logic when fetching external import maps. On event of this error, if defined, the script in the onerror attribute will be executed.

<script type="systemjs-importmap" onerror="handleError()" src="unable-to-reach/importmap.json"></script>

This is addressing this issue.

@github-actions
Copy link

github-actions bot commented Apr 28, 2021

File size impact

Merging onerror-callback-support into master will impact 3 files in 2 groups.

browser (2/2)
File raw gzip brotli Event
dist/s.min.js +42 (7,555) +14 (2,978) +17 (2,706) modified
dist/system.min.js +42 (11,705) +13 (4,488) +12 (4,042) modified
Total size impact +84 (19,260) +27 (7,466) +29 (6,748)
node (1/1)
File raw gzip brotli Event
dist/system-node.cjs +92 (201,498) +19 (52,164) +68 (43,857) modified
Total size impact +92 (201,498) +19 (52,164) +68 (43,857)
extras (0/8)

No impact on files in extras group.

Generated by file size impact during size-impact#793595550

try {
script.onerror();
} catch(callbackErr) {
callbackErr.message = 'Error during onerror script for systemjs-importmap: ' + script.src + '\n' + callbackErr.message;
Copy link
Member

Choose a reason for hiding this comment

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

This error should be obtainable from the stack anyway, so let's remove this try-catch wrapper rather. Or did you specifically find the stack didn't have the information needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stack has the required info, I will remove the try-catch, thanks for the quick review 👍

Copy link
Member

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the PR. Just the one change request.

@guybedford guybedford merged commit f34cdc6 into systemjs:master Apr 28, 2021
@kykwak kykwak deleted the onerror-callback-support branch April 28, 2021 18:35
@kykwak kykwak mentioned this pull request Apr 28, 2021
10 tasks
@joeldenning
Copy link
Collaborator

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

3 participants