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

fix(hmr-plugin): remove @angularclass/hmr peer dependency #1205

Merged
merged 18 commits into from
Aug 19, 2019

Conversation

splincode
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

image

We depend on a package that has not been updated for 2 years, while using only 2 functions from there.

What is the new behavior?

Now we use our own functionality and it works

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@splincode
Copy link
Member Author

@markwhitfeld @arturovt ping

@splincode splincode requested a review from markwhitfeld August 11, 2019 08:13
packages/store/src/store.ts Outdated Show resolved Hide resolved
packages/store/src/host-environment/host-environment.ts Outdated Show resolved Hide resolved
packages/hmr-plugin/src/hmr-manager.ts Outdated Show resolved Hide resolved
packages/hmr-plugin/src/hmr-manager.ts Outdated Show resolved Hide resolved
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

We gave feedback on the PR, and then when I look at the changes there is a whole lot of new stuff in the PR unrelated to the feedback and not entirely necessary for the PR.
This has happened quite often and is a bit frustrating. Please don't do this. It makes the review process tedious because we have to start again as reviewers.

packages/hmr-plugin/src/symbols.ts Outdated Show resolved Hide resolved
@splincode splincode requested a review from markwhitfeld August 16, 2019 20:14
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Thanks for reverting those unrelated changes.
Some changes requested below...

integration/main.browser.ts Show resolved Hide resolved
packages/hmr-plugin/src/hmr-bootstrap.ts Outdated Show resolved Hide resolved
packages/hmr-plugin/src/hmr-manager.ts Outdated Show resolved Hide resolved
packages/hmr-plugin/src/symbols.ts Outdated Show resolved Hide resolved
@splincode splincode requested a review from markwhitfeld August 18, 2019 18:07
@splincode
Copy link
Member Author

@markwhitfeld @arturovt done

@arturovt
Copy link
Member

arturovt commented Aug 18, 2019

Do you have to remove this package from the root package.json?

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Nicely done

@splincode splincode requested a review from arturovt August 19, 2019 18:35
Copy link
Member

@arturovt arturovt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

LGTM... although I'm not sure why there are all those other unrelated package upgrades in those last commits?

@arturovt
Copy link
Member

@markwhitfeld rebases after Renovate's updates

@splincode splincode merged commit 24d5e08 into master Aug 19, 2019
@markwhitfeld
Copy link
Member

@arturovt I was referring to the changes in @splincode's 'fix' commits. Was this just yarn being eager?

@markwhitfeld markwhitfeld deleted the fix/hmr-plugin branch August 19, 2019 20:10
@arturovt
Copy link
Member

I guess it were merge conflicts :)

@markwhitfeld markwhitfeld added this to the 3.5.x milestone Aug 29, 2019
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.

3 participants