Skip to content

WIP: Add vm polyfill for browser#40

Open
a-mountain wants to merge 1 commit into
metarhia:masterfrom
a-mountain:add-polyfill-vm
Open

WIP: Add vm polyfill for browser#40
a-mountain wants to merge 1 commit into
metarhia:masterfrom
a-mountain:add-polyfill-vm

Conversation

@a-mountain

@a-mountain a-mountain commented Nov 24, 2020

Copy link
Copy Markdown

Allows run script in iframe with your context
Add tests

  • tests and linter show no problems (npm run t)
  • tests are added/updated for bug fixes and new features
  • code is properly formatted (npm run fmt)

Comment thread browser-test/vm.js Outdated
Comment thread browser-test/vm.js Outdated
Comment thread lib/browser-vm/context.js Outdated
Comment thread lib/browser-vm/context.js Outdated
Comment thread lib/browser-vm/context.js Outdated
Comment thread lib/browser-vm/vm.js Outdated
Comment thread VM BROWSER.md
@@ -0,0 +1,193 @@
# VM BROWSER

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename to doc/vm-browser.md and format with prettier.

Comment thread dist/vm.js Outdated
Comment on lines +54 to +63
.map((error) => ({
name: error.name,
newError: class ErrorWithFilename extends error {
constructor(message, filename, lineNumber) {
super(message, filename, lineNumber);
this.filename = errorFilename;
}
},
}))
.forEach((v) => (contentWindow[v.name] = v.newError));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer for..of

Comment thread dist/vm.js Outdated
Comment on lines +13 to +16
const throwEvalError = () => {
const msg = 'Code generation from strings disallowed for this context';
throw new EvalError(msg);
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const throwEvalError = () => {
const msg = 'Code generation from strings disallowed for this context';
throw new EvalError(msg);
};
const ERR_EVAL = 'Code generation from strings disallowed for this context';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use throw new Error(ERR_EVAL); from all other places

Comment thread dist/vm.js Outdated
constructor(code, options = {}) {
this.code = code;
if (isFilename(options)) {
options = makeOptionsFromFilename(options);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't rewrite arguments

@tshemsedinov tshemsedinov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also please rebase on master

Comment thread dist/vm.js
element.style.display = 'none';
element.id = id;
element.name = name;
element.className = IFRAME_CLASS_DEFAULT;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use hardcode, Luke

Comment thread dist/vm.js
Comment on lines +42 to +49
const wrappedErrors = errors.map((error) => ({
name: error.name,
newError: class ErrorWithFilename extends error {
constructor(message, filename, lineNumber) {
super(message, filename, lineNumber);
this.filename = errorFilename;
}
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const wrappedErrors = errors.map((error) => ({
name: error.name,
newError: class ErrorWithFilename extends error {
constructor(message, filename, lineNumber) {
super(message, filename, lineNumber);
this.filename = errorFilename;
}
},
const wrappedErrors = errors.map((ErrorClass) => ({
name: ErrorClass.name,
newError: class ErrorWithFilename extends ErrorClass {
constructor(message, filename, lineNumber) {
super(message, filename, lineNumber);
this.filename = errorFilename;
}
},

Comment thread dist/vm.js
}
},
}));
for (const wrappedError of wrappedErrors) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Avoid double loops over error classes collection

Comment thread dist/vm.js
}
},
}));
for (const wrappedError of wrappedErrors) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for (const wrappedError of wrappedErrors) {
for (const ErrorClass of wrappedErrors) {

Comment thread dist/vm.js
};

const createIframe = (document, context, options) => {
const id = `iframe-vm:${Date.now()}`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Counter

Comment thread dist/vm.js
Comment on lines +85 to +104
return {
id,
deleteFromDocument() {
if (isAttachedToDocument()) {
document.body.removeChild(element);
}
},
updateContext() {
if (isAttachedToDocument()) {
for (const [key, value] of Object.entries(contentWindow)) {
if (isContextKey(key) || canAddNewKey(key)) {
context[key] = value;
}
}
}
},
runScript(script) {
return runScript(script);
},
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Class

Comment thread dist/vm.js
Comment on lines +123 to +124
return {
updateContext() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Class

Comment thread dist/vm.js
Comment on lines +154 to +157
} catch (e) {
deleteIframe(iframe);
throw e;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
} catch (e) {
deleteIframe(iframe);
throw e;
}
} catch (error) {
deleteIframe(iframe);
throw error;
}

@a-mountain
a-mountain force-pushed the add-polyfill-vm branch 2 times, most recently from aa135e3 to fd357e8 Compare March 8, 2023 14:18
@tshemsedinov tshemsedinov changed the title Add vm polyfill for browser WIP: Add vm polyfill for browser Dec 18, 2024
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.

2 participants