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

Add vm polyfill for browser #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-mountain
Copy link

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

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)

browser-test/vm.js Outdated Show resolved Hide resolved
browser-test/vm.js Outdated Show resolved Hide resolved
lib/browser-vm/context.js Outdated Show resolved Hide resolved
lib/browser-vm/context.js Outdated Show resolved Hide resolved
lib/browser-vm/context.js Outdated Show resolved Hide resolved
lib/browser-vm/vm.js Outdated Show resolved Hide resolved
VM BROWSER.md Outdated
@@ -0,0 +1,193 @@
# VM BROWSER
Copy link
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.

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
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

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
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
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

dist/vm.js Outdated
constructor(code, options = {}) {
this.code = code;
if (isFilename(options)) {
options = makeOptionsFromFilename(options);
Copy link
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

Copy link
Member

@tshemsedinov tshemsedinov left a comment

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

element.style.display = 'none';
element.id = id;
element.name = name;
element.className = IFRAME_CLASS_DEFAULT;
Copy link
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 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
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;
}
},

}
},
}));
for (const wrappedError of wrappedErrors) {
Copy link
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

}
},
}));
for (const wrappedError of wrappedErrors) {
Copy link
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) {

};

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

Choose a reason for hiding this comment

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

Counter

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
Member

Choose a reason for hiding this comment

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

Class

Comment on lines +123 to +124
return {
updateContext() {
Copy link
Member

Choose a reason for hiding this comment

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

Class

Comment on lines +154 to +157
} catch (e) {
deleteIframe(iframe);
throw e;
}
Copy link
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;
}

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

2 participants