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

refactor: Rename imported symbols #179

Merged
merged 3 commits into from
Mar 25, 2020

Conversation

ExE-Boss
Copy link
Contributor

I’ve waited with opening this until after #166 was done to avoid merge conflicts.

This frees up the impl variable for use in #158 and also makes it clearer that the current impl variable refers to implSymbol.

this.requires.addRaw("impl", "utils.implSymbol");
this.requires.addRaw("ctorRegistry", "utils.ctorRegistrySymbol");
this.requires.addRaw(`{ implSymbol, ctorRegistrySymbol }`, "utils");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve changed this to use object destructuring to match how alias imports are done in JSDOM.

@ExE-Boss ExE-Boss force-pushed the refactor/rename-imported-symbols branch from 1029610 to 839fa5e Compare March 23, 2020 19:10
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Looks good otherwise.

README.md Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss requested a review from TimothyGu March 23, 2020 23:27
@TimothyGu TimothyGu force-pushed the refactor/rename-imported-symbols branch from e3ace27 to 96794fa Compare March 25, 2020 21:35
@TimothyGu
Copy link
Member

Rebased and fixed some issues that came up in rebasing (96794fa).

@TimothyGu TimothyGu merged commit 2024ca2 into jsdom:master Mar 25, 2020
@TimothyGu TimothyGu deleted the refactor/rename-imported-symbols branch March 25, 2020 21:37
@ExE-Boss
Copy link
Contributor Author

I had originally fixed that as part of 42e8cd5.

@TimothyGu
Copy link
Member

TimothyGu commented Mar 25, 2020

It’s possible that rebasing destroyed the merge. For future could you please rebase rather than merge

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