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.

Sign up for GitHub

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

Jump to bottom

Switch to npm #117

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Switch to npm #117

merged 1 commit into from
Apr 12, 2021

Conversation

ferferga
Copy link
Member

Changes this project package management from yarn to npm, as it's being done in all the organization repositories.

Major reasons for switching:

  • All the yarn's advantages over npm are non-existent today
  • Yarn introduced some breaking changes in 2.x that caused controversies and avoid us to upgrade directly, which means we are stuck in a version in maintenance mode. So far, all the npm major updates could be updated like a charm, and it's likely they will commit to it. The same can't be said about yarn.
  • Npm comes with the node installation, so it's frictionless for the user
  • Npm is more mainstream and all the package maintainers add instructions with how to use it to install their packages. The same doesn't apply to yarn. Although adapting the commands to yarn is trivial, for new contributors it might cause some friction.

Extra

  • Added dependabot for dependency updates in GitHub Actions

@ferferga ferferga force-pushed the deyarn branch 3 times, most recently from e93a1dc to 72158e2 Compare March 10, 2021 17:48
hawken93
hawken93 reviewed Mar 26, 2021
View reviewed changes
src/components/commandHandler.ts
@@ -24,7 +24,7 @@ import { playbackManager } from './playbackManager';
import { DocumentManager } from './documentManager';

export abstract class CommandHandler {
private static playerManager: cast.framework.PlayerManager;
private static playerManager: framework.PlayerManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

just a question here, I don't fully understand why "cast" is implicit here..

Copy link
Member Author

Choose a reason for hiding this comment

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

Found this issue, basically the typings are not exported in the index, but in framework.

Shouldn't affect runtime though as it's all TypeScript abstraction as usual :)

Good catch!

hawken93
hawken93 approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@hawken93 hawken93 left a comment

Choose a reason for hiding this comment

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

I didn't see anything that I wanted to do differently. LGTM :)

@ferferga
Copy link
Member Author

I was able to test this today on an ATV device and I had no issues with the switch, so I'm merging this :)

@ferferga ferferga merged commit 2c3fcb5 into master Apr 12, 2021
@ferferga ferferga deleted the deyarn branch April 12, 2021 19:14
@ferferga ferferga mentioned this pull request Apr 12, 2021
Merged
@YouKnowBlom YouKnowBlom mentioned this pull request Jul 7, 2021
Closed
@YouKnowBlom YouKnowBlom mentioned this pull request Dec 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawken93 hawken93 hawken93 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ferferga @hawken93