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

Fix launching extension in safari #10908

Merged
merged 10 commits into from
Jul 25, 2022
Merged

Fix launching extension in safari #10908

merged 10 commits into from
Jul 25, 2022

Conversation

DonJayamanne
Copy link
Contributor

Fixes #10621

@DonJayamanne DonJayamanne requested a review from a team as a code owner July 25, 2022 01:50
return;
}
const textToReplace = `: setImmediate;`;
const textToReplaceWith = `: typeof setImmediate === 'function' ? setImmediate : setTimeout;`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will submit PR upstream, setImmediate only runs in node. however we run our extensions in webworker, and jupyter code is written to run either in browser or node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will look into (tomorrow) using the setimmediate pollyfill npm package as an alternative to modifying such scripts.

const contents = dedent`
'use strict';

exports.python = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other reg exes are not required and that causes issues in loading extension.

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

const { launch } = require('./launchWebUtils');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ability to launch extension in vscode web locally

@@ -66,7 +66,7 @@ export function extractRequireConfigFromWidgetEntry(baseUrl: Uri, widgetFolderNa
// the config entry is js, and not json.
// We cannot eval as thats dangerous, and we cannot use JSON.parse either as it not JSON.
// Lets just extract what we need.
configStr = stripComments(configStr);
configStr = stripComments(configStr, { language: 'python' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Be explicit about the fact that we're only stripping js code comments

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #10908 (d0b8256) into main (1262e7c) will decrease coverage by 0%.
The diff coverage is n/a.

❗ Current head d0b8256 differs from pull request most recent head 39bc902. Consider uploading reports for the commit 39bc902 to get more accurate results

@@          Coverage Diff           @@
##            main   #10908   +/-   ##
======================================
- Coverage     63%      63%   -1%     
======================================
  Files        482      482           
  Lines      33831    33831           
  Branches    5515     5515           
======================================
- Hits       21468    21445   -23     
- Misses     10319    10345   +26     
+ Partials    2044     2041    -3     
Impacted Files Coverage Δ
src/kernels/common/chainingExecuteRequester.ts 66% <0%> (-17%) ⬇️
src/notebooks/debugger/debugger.ts 66% <0%> (-12%) ⬇️
src/kernels/execution/cellExecution.ts 72% <0%> (-8%) ⬇️
src/notebooks/debugger/debuggingManagerBase.ts 69% <0%> (-7%) ⬇️
...kernels/jupyter/preferredRemoteKernelIdProvider.ts 92% <0%> (-5%) ⬇️
src/platform/common/cancellation.ts 64% <0%> (-4%) ⬇️
src/notebooks/debugger/kernelDebugAdapterBase.ts 76% <0%> (-3%) ⬇️
src/notebooks/debugger/debuggerVariables.ts 67% <0%> (-2%) ⬇️
src/platform/msrCrypto/msrCrypto.js 16% <0%> (+<1%) ⬆️
src/platform/common/utils/localize.ts 76% <0%> (+<1%) ⬆️
... and 3 more

@@ -125,7 +126,77 @@ function fixJupyterLabRenderers() {
}
}

function fixJupyterLabFuture() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need like, a comment here? Or a link to the PR upstream?

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

Could maybe use a comment in postInstall.js

@DonJayamanne
Copy link
Contributor Author

@IanMatthewHuff @rchiodo please re-review, updated the solution

@DonJayamanne DonJayamanne merged commit ca46927 into main Jul 25, 2022
@DonJayamanne DonJayamanne deleted the fixWebLaunchSafari branch July 25, 2022 22:59
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.

Jupyter can not get activated on Safari
4 participants