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: removed underscore from protected methods in DefaultCrudRep… #2112

Conversation

clayrisser
Copy link
Contributor

@clayrisser clayrisser commented Dec 4, 2018

Removed underscore from protected methods

Fixes #2091

Checklist

  • npm test passes on your machine
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

protected _createBelongsToAccessorFor<Target extends Entity, TargetId>(
relationName: string,
targetRepoGetter: Getter<EntityCrudRepository<Target, TargetId>>,
): BelongsToAccessor<Target, ID> {
return this.createBelongsToAccessorFor(relationName, targetRepoGetter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it make sense to log the deprecation warning ?
What's the overall project strategy to deal with this ?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. We don't have a deprecation strategy yet.

I guess the question is how much we want to force our users to update their code.

A slow process:

  1. Soft deprecations (docs only)
  2. Hard deprecation (warnings printed at runtime) - maybe wait until the next semver-major?
  3. API removed (in a semver-major release)

A faster process:

  1. Hard deprecation (warnings printed at runtime)
  2. API removed (in a semver-major release)

In the past, we were using the fast process. From what I heard about React, their users love their slower process for API deprecations, maybe we can do the same in LoopBack?

Implementation wise, we have had good experience using depd module for dealing with deprecation warnings in LoopBack 2.x/3.x

Copy link
Member

Choose a reason for hiding this comment

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

On the second thought, since this is a simple rename that has a trivial upgrade path, I am fine with a hard deprecation from the beginning.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd like to get at least one more approval from @strongloop/loopback-maintainers before proceeding with landing.

@bajtos bajtos force-pushed the codejamninja/no-underscore-default-repository branch from b0d1b81 to ad4d70f Compare December 6, 2018 16:21
@bajtos
Copy link
Member

bajtos commented Dec 6, 2018

Rebased on top of the latest master, squashed the commits into a single one and slightly improved the commit message. Let's wait for CI results before landing.

@bajtos bajtos merged commit 40973a8 into loopbackio:master Dec 6, 2018
@bajtos
Copy link
Member

bajtos commented Dec 6, 2018

Landed 🎉 Thank you @codejamninja for the contribution! ❤️

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.

None yet

5 participants