fix: proxy-less env support #171
Conversation
Thank you a lot for your review! Will be happy to work on that today or tomorrow :) |
src/v8n.js
Outdated
| return newContext._applyRule(availableRules[prop], prop)(...args); | ||
| }; | ||
| }); | ||
| const buildRuleSet = ruleSet => |
There was a problem hiding this comment.
To avoid side effects this should accept the context as a parameter and return it. This is currently modifying a variable in place and is not very readable.
Also: Does this name make sense? This adds a rule set to the context and not only builds it. Just as a side note.
src/v8n.js
Outdated
| }); | ||
| const buildRuleSet = ruleSet => | ||
| Object.keys(ruleSet).forEach(prop => { | ||
| context[prop] = (...args) => { |
There was a problem hiding this comment.
Where is this variable from? Should be a function argument for buildRuleSet.
src/v8n.js
Outdated
| }; | ||
| }); | ||
| buildRuleSet(availableRules); | ||
| buildRuleSet(customRules); |
There was a problem hiding this comment.
This changes context but that's not readable.
|
@brfrth I'm not sure if that's what you meant or I missed something, waiting for the further feedback |
|
I’ll read over this a little later. Thanks for the work. |
src/v8n.js
Outdated
| return newContext._applyRule(ruleSet[prop], prop)(...args); | ||
| }; | ||
| }); | ||
|
|
||
| buildRuleSet(availableRules); | ||
| buildRuleSet(customRules); | ||
| addRuleSet(availableRules, context); |
There was a problem hiding this comment.
The returned, changed context should be reassigned here. otherwise the context variable is changed below and it's not clear where. So it should be clearly reassigned.
|
@brfrth did you have a moment to take a look? |
|
@NoemiRozpara I've tested this PR using JINT (ES5.1 spec only implementation for .Net). It throws an error that When I have a second to get into it a bit more I'll try and find the issue. Also, thank you! |
|
@NoemiRozpara Sorry for taking so long. The reassignment looks fine now. The error that @ev-akarel mentioned would need to be looked at though. I can't remove the merge block until it is. |
|
@NoemiRozpara @brfrth E.g. const addRuleSet = (ruleSet, targetContext) => {
Object.keys(ruleSet).forEach(prop => {
targetContext[prop] = (...args) => {
const newContext = proxyContextEs5(targetContext._clone());
const contextWithRuleApplied = newContext._applyRule(
ruleSet[prop],
prop
)(...args);
return contextWithRuleApplied;
};
});
return targetContext;
};One last question: Does |
|
Closing this due to the lack of activity from @NoemiRozpara. Please feel free to comment here if you merge the changes @ev-akarel suggested. |
Description
Commit picked from https://github.com/bassarisse/v8n/ and applied to the most recent version of the library. Provides IE11 support.
Type of change
Checklist:
masterUnreleasedsection of the CHANGELOG