fix(core): prevent renaming global providers and modules in the repl #9720
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
micalevisk
commented
Jun 2, 2022
micalevisk
changed the title
micalevisk
force-pushed
the
feat/repl-fix-injections
branch
from
4487924
to
b1d3d48
Compare
by marking them as a read-only properties of `globalThis` obj
micalevisk
force-pushed
the
feat/repl-fix-injections
branch
from
b1d3d48
to
a2732a4
Compare
|
I'm thinking if is't possible to implement this while keeping the test suite healthy as they need to touch
(due to those Close this if you know how to handle that |
Pull Request Test Coverage Report for Build 60be6c3c-e300-4424-82bf-b2b719b4098f
💛 - Coveralls |
micalevisk
force-pushed
the
feat/repl-fix-injections
branch
2 times, most recently
from
bf25d6b
to
ed766bb
Compare
micalevisk
commented
Jun 4, 2022
packages/core/repl/repl.ts
Outdated
| import { ReplContext } from './repl-context'; | ||
| import { ReplLogger } from './repl-logger'; | ||
|
|
||
| function copyInto(target, source): void { |
There was a problem hiding this comment.
we need this utility because Object.assign won't copy property's descriptors
I could extract this to another file
micalevisk
force-pushed
the
feat/repl-fix-injections
branch
from
ed766bb
to
2c4aa9f
Compare
Tony133
approved these changes
Jun 5, 2022
micalevisk
commented
Jun 5, 2022
12 tasks
|
LGTM |
|
Great job @micalevisk! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
and I believe the above shouldn't be allowed as there's no way (I guess) to recover those classes references
What is the new behavior?
mark those injected classes as a read-only properties on
replServer.contextobj. There's no need to useglobalThisanymore 🎉 as of nowReplContextisn't aware of the server context that it will be used. Instead,replServermust pollute their global scopeThe goal here is to improve UX. Now users couldn't change those references accidentally
Does this PR introduce a breaking change?