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

[Try] Async stan #27155

Closed
wants to merge 1 commit into from
Closed

[Try] Async stan #27155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/stan/src/atom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@ export const createAtom = ( initialValue, config = {} ) => () => {
type: 'root',
set( newValue ) {
value = newValue;
listeners.forEach( ( l ) => l() );
return Promise.all( Array.from( listeners ).map( ( l ) => l() ) );
},
get() {
return value;
return Promise.resolve( value );
},
async resolve() {
return value;
return Promise.resolve( value );
},
subscribe( listener ) {
listeners.add( listener );
return () => {
return Promise.resolve( () => {
listeners.delete( listener );
};
} );
},
isResolved: true,
};
Expand Down
90 changes: 32 additions & 58 deletions packages/stan/src/derived.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const createDerivedAtom = ( resolver, updater = noop, config = {} ) => (
/**
* @type {any}
*/
let value = null;
const valueContainer = { value: null };

/**
* @type {Set<() => void>}
Expand All @@ -60,16 +60,15 @@ export const createDerivedAtom = ( resolver, updater = noop, config = {} ) => (

const dependenciesUnsubscribeMap = new WeakMap();

const notifyListeners = () => {
listeners.forEach( ( l ) => l() );
};
const notifyListeners = async () =>
Promise.all( Array.from( listeners ).map( ( l ) => l() ) );

const refresh = () => {
const refresh = async () => {
if ( listeners.size > 0 ) {
if ( config.isAsync ) {
resolveQueue.add( context, resolve );
} else {
resolve();
await resolve();
}
} else {
isListening = false;
Expand All @@ -79,44 +78,33 @@ export const createDerivedAtom = ( resolver, updater = noop, config = {} ) => (
/**
* @param {WPAtomState<any>} atomState
*/
const addDependency = ( atomState ) => {
const addDependency = async ( atomState ) => {
if ( ! dependenciesUnsubscribeMap.has( atomState ) ) {
dependenciesUnsubscribeMap.set(
atomState,
atomState.subscribe( refresh )
await atomState.subscribe( refresh )
);
}
};

const resolve = () => {
const resolve = async () => {
/**
* @type {(WPAtomState<any>)[]}
*/
const updatedDependencies = [];
const updatedDependenciesMap = new WeakMap();
let result;
const unresolved = [];
try {
result = resolver( {
get: ( atom ) => {
const atomState = registry.__unstableGetAtomState( atom );
// It is important to add the dependency as soon as it's used
// because it's important to retrigger the resolution if the dependency
// changes before the resolution finishes.
addDependency( atomState );
updatedDependenciesMap.set( atomState, true );
updatedDependencies.push( atomState );
if ( ! atomState.isResolved ) {
unresolved.push( atomState );
}
return atomState.get();
},
} );
} catch ( error ) {
if ( unresolved.length === 0 ) {
throw error;
}
}
const result = await resolver( {
get: async ( atom ) => {
const atomState = registry.__unstableGetAtomState( atom );
// It is important to add the dependency as soon as it's used
// because it's important to retrigger the resolution if the dependency
// changes before the resolution finishes.
await addDependency( atomState );
updatedDependenciesMap.set( atomState, true );
updatedDependencies.push( atomState );
return await atomState.get();
},
} );

function removeExtraDependencies() {
const removedDependencies = dependencies.filter(
Expand All @@ -135,42 +123,28 @@ export const createDerivedAtom = ( resolver, updater = noop, config = {} ) => (
/**
* @param {any} newValue
*/
function checkNewValue( newValue ) {
if ( unresolved.length === 0 && newValue !== value ) {
value = newValue;
async function checkNewValue( newValue ) {
if ( newValue !== valueContainer.value ) {
valueContainer.value = newValue;
isResolved = true;
notifyListeners();
await notifyListeners();
}
}

if ( result instanceof Promise ) {
// Should make this promise cancelable.
result
.then( ( newValue ) => {
removeExtraDependencies();
checkNewValue( newValue );
} )
.catch( ( error ) => {
if ( unresolved.length === 0 ) {
throw error;
}
removeExtraDependencies();
} );
} else {
removeExtraDependencies();
checkNewValue( result );
}
removeExtraDependencies();
await checkNewValue( result );
return result;
};

return {
id: config.id,
type: 'derived',
get() {
async get() {
if ( ! isListening ) {
isListening = true;
resolve();
await resolve();
}
return value;
return valueContainer.value;
},
async set( action ) {
await updater(
Expand All @@ -182,10 +156,10 @@ export const createDerivedAtom = ( resolver, updater = noop, config = {} ) => (
);
},
resolve,
subscribe( listener ) {
async subscribe( listener ) {
Copy link
Contributor Author

@adamziel adamziel Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unsubscribe callback should be available right away so maybe this should return synchronously with an array of promise and unsubscribe callback?

if ( ! isListening ) {
isListening = true;
resolve();
await resolve();
}
listeners.add( listener );
return () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/stan/src/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,16 @@ export const createAtomRegistry = ( onAdd = noop, onDelete = noop ) => {
// This shouldn't be necessary since we rely on week map
// But the legacy selectors/actions API requires us to know when
// some atoms are removed entirely to unsubscribe.
delete( atom ) {
async delete( atom ) {
if ( isAtomFamilyItem( atom ) ) {
const {
config,
key,
} = /** @type {WPAtomFamilyItem<any>} */ ( atom );
return familyRegistry.deleteAtomFromFamily( config, key );
}
const atomState = atoms.get( atom );
atoms.delete( atom );
const atomState = await atoms.get( atom );
await atoms.delete( atom );
onDelete( atomState );
},
};
Expand Down
22 changes: 11 additions & 11 deletions packages/stan/src/test/atom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,25 @@
import { createAtomRegistry, createAtom } from '../';

describe( 'atoms', () => {
it( 'should allow getting and setting atom values', () => {
it( 'should allow getting and setting atom values', async () => {
const count = createAtom( 1 );
const registry = createAtomRegistry();
expect( registry.get( count ) ).toEqual( 1 );
registry.set( count, 2 );
expect( registry.get( count ) ).toEqual( 2 );
expect( await registry.get( count ) ).toEqual( 1 );
await registry.set( count, 2 );
expect( await registry.get( count ) ).toEqual( 2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need all the async/wait for the getters for sync atoms and subscriptions? Note that even if the behavior is sync, JS will make it async and only resolve on the next tick and this breaks the editor (and useSelect) in various ways. It is important that sync selectors stay sync and resolve right away.

Copy link
Contributor Author

@adamziel adamziel Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, I’ll think some more about this on Monday. It seems like Recoil separates atoms and selectors, and only the latter may be async - maybe that would be a better way to go, our API is already pretty similar. I wonder how it handles errors thrown on the intersection of the two though, like propagation of the atom value to async selectors

} );

it( 'should allow subscribing to atom changes', () => {
it( 'should allow subscribing to atom changes', async () => {
const count = createAtom( 1 );
const registry = createAtomRegistry();
const listener = jest.fn();
registry.subscribe( count, listener );
expect( registry.get( count ) ).toEqual( 1 );
registry.set( count, 2 );
await registry.subscribe( count, listener );
expect( await registry.get( count ) ).toEqual( 1 );
await registry.set( count, 2 );
expect( listener ).toHaveBeenCalledTimes( 1 );
expect( registry.get( count ) ).toEqual( 2 );
registry.set( count, 3 );
expect( await registry.get( count ) ).toEqual( 2 );
await registry.set( count, 3 );
expect( listener ).toHaveBeenCalledTimes( 2 );
expect( registry.get( count ) ).toEqual( 3 );
expect( await registry.get( count ) ).toEqual( 3 );
} );
} );
Loading