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

feat: enable HMR of persistentReducedStreams (opt in) #555

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
interface ConfigMap {
enableHotModuleReload: boolean;
}
type ConfigKeys = keyof ConfigMap;

function createGlobalConfig() {
const config: ConfigMap = {
enableHotModuleReload: false,
};

return {
setProperty: <Key extends ConfigKeys>(key: Key, value: ConfigMap[Key]) => {
config[key] = value;
},
getProperty: <Key extends ConfigKeys>(key: Key) => {
return config[key];
},
};
}

export const rxBeachConfig = createGlobalConfig();
29 changes: 26 additions & 3 deletions src/persistentReducedStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
ReducedStreamOptions,
reducedStream as reducedStreamInternal,
} from './internal/reducedStream';
import { rxBeachConfig } from './config';

/**
* Creates and registers a persistent reduced state stream
Expand Down Expand Up @@ -48,9 +49,31 @@ export const persistentReducedStream = <State>(
tag(name)
);

const stream = new ObservableState(name, source$, initialState);
try {
Copy link

@chriskr chriskr Dec 6, 2022

Choose a reason for hiding this comment

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

Why are you using try-catch instead of module.hot?

That error detection seems the wrong trigger for hot reloading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question!

This file is not the file that is being hot reloaded, it is the file that uses this function that will be reloaded. So that file will run multiple times and register a persistentReducedStream each time, which typically is a duplicate reducer error that needs to be handled in order to allow HMR to happen.

Copy link

Choose a reason for hiding this comment

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

I see. Using that error type is an indirect pattern, that is basically not ideal. I think it would be better to add some info in dev mode to the subscriber that this is a subscription triggered by a hot reloading to make the reason for the re-subscription explicit.

const stream = new ObservableState(name, source$, initialState);
stateStreamRegistry.register(stream);
return stream;
} catch (error) {
if (
rxBeachConfig.getProperty('enableHotModuleReload') &&
error instanceof Error &&
error.message.startsWith('Duplicate stream name: ')
) {
// The following enables Hot Module Reloading (HMR) of persistentReducedStream by
// handling the "duplicate stream" error that is thrown when reusing a stream name.
console.warn(
`[HMR] Replacing stream "${name}", if you see this warning when your ` +
`application boots the first time you have used the same name twice!`
);
const prevState =
stateStreamRegistry.streams.get(name)?.state ?? initialState;
const newStream = new ObservableState(name, source$, prevState);

stateStreamRegistry.register(stream);
stateStreamRegistry.unregister(name);
stateStreamRegistry.register(newStream);
return newStream;
}

return stream;
throw error;
}
};
16 changes: 16 additions & 0 deletions src/stateStreamRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@ export class StateStreamRegistry {
this.streams.set(stream.name, stream);
}

/**
* Unregister a stream from the registry
*
* @param name The name of the stream
* @returns void
*/
unregister(name: string) {
var stream = this.streams.get(name)
if (!stream) {
return;
}

this.streams.delete(name)
stream.unsubscribe();
};

/**
* Start the registered reduced state streams.
*
Expand Down