Deprecates adapter & persister name in favor of id
#310
Conversation
| @@ -228,7 +228,8 @@ export default function adapterTests() { | |||
| expect(resolved).to.have.members([1, 2, 3]); | |||
| }); | |||
|
|
|||
| it('should work with CORS requests', async function() { | |||
| // NOTE: test very unstable because of typicode.com being down | |||
| it.skip('should work with CORS requests', async function() { | |||
There was a problem hiding this comment.
This test has been failing off and on for a couple months now, disabling.
jasonmit
force-pushed
the
u/jasonmit/deprecate-name
branch
from
215d898
to
b10c746
Compare
| return 'rest'; | ||
| } | ||
|
|
||
| static get name() { |
There was a problem hiding this comment.
All of these name getter proxies need to sit on the persister/adapter classes instead of base classes, reason:
class Foo {
static get name() {
return 'foo';
}
}
class Bar extends Foo {}
// "foo" not "Foo"
Foo.name;
// "Bar" not "foo" - always the class/fn `name`, never super's `name`
Bar.name;
| this.polly.pause(); | ||
| await this.fetchRecord({ method: 'DELETE' }); | ||
| this.polly.play(); | ||
| if (this.polly) { |
There was a problem hiding this comment.
Possible for test suite to fail before this.polly is set and made the stderr annoying to scroll through since we always assume it was.
There was a problem hiding this comment.
Might be good to add an inline comment about this.
jasonmit
changed the title
name in favor of idname in favor of id
packages/@pollyjs/core/src/polly.js
Outdated
| @@ -3,7 +3,7 @@ import { MODES, assert } from '@pollyjs/utils'; | |||
| import { version } from '../package.json'; | |||
|
|
|||
| import Logger from './-private/logger'; | |||
| import Container from './-private/container'; | |||
| import Container, { nameOfFactory } from './-private/container'; | |||
There was a problem hiding this comment.
iirc rollup has had issues supporting default and named exports. Might need to switch them both to a named export.
There was a problem hiding this comment.
and our tests still pass with this issue?
| @@ -1,7 +1,19 @@ | |||
| import { assert } from '@pollyjs/utils'; | |||
|
|
|||
| export function nameOfFactory(Factory) { | |||
There was a problem hiding this comment.
Should this be idOfFactory or factoryId?
| static get name() { | ||
| // NOTE: deprecated in 4.1.0 but proxying since it's possible "core" is behind | ||
| // and therefore still referencing `name`. Remove in 5.0.0 | ||
| return this.id; |
There was a problem hiding this comment.
Should we just release a new major version instead of having to deal with all of these deprecations?
There was a problem hiding this comment.
It's a minor change imo and will help anyone with custom adapters to migrate without reading docs.
| this.polly.pause(); | ||
| await this.fetchRecord({ method: 'DELETE' }); | ||
| this.polly.play(); | ||
| if (this.polly) { |
There was a problem hiding this comment.
Might be good to add an inline comment about this.
packages/@pollyjs/core/src/polly.js
Outdated
| @@ -262,7 +262,7 @@ export default class Polly { | |||
|
|
|||
| if (typeof nameOrFactory === 'function') { | |||
| container.register(nameOrFactory); | |||
| adapterName = nameOrFactory.name; | |||
| adapterName = nameOfFactory(nameOrFactory); | |||
jasonmit
force-pushed
the
u/jasonmit/deprecate-name
branch
from
968b969
to
0311c26
Compare
The replacement of the static `name` getter for `Adapter` and `Persister` in Netflix#310 broke custom implementations of these classes because `id` needed to be overridden. To retain backwards compatibility we remove the requirement for `id` to be overridden. Instead we delegate to `name` by default. This does not affect the in-tree implementations.
Done in a backwards compatible way. In the next major we can remove the deprecations.
Fixes #309