twilio-professional-services / flex-project-template

A starting point for Twilio Flex projects, providing management strategies for various types of artifacts and distributed development
https://twilio-professional-services.github.io/flex-project-template/
Apache License 2.0
63 stars 67 forks source link

Add and use loaded features array #563

Closed dremin closed 1 month ago

dremin commented 3 months ago

Summary

Features may need to be aware of certain other features being enabled. For example, pause-recording needs to know if dual-channel-recording is enabled to determine the API to use.

Unfortunately, simply checking if a feature is enabled is not perfect--if a feature has been removed, for example, it is possible that the flex-config lists it as enabled even though the feature does not exist.

This PR adds a simple array to the config helper that is populated by the feature-loader after each feature loads. This provides template features with a list of the actually-loaded features, rather than simply reading the config.

There is a bit of a catch with this, of course--if we haven't finished loading features, the array is incomplete. This means that the array should only be accessed after feature loading is complete.

Checklist

github-actions[bot] commented 3 months ago

0 ESLint error(s) and 0 ESLint warning(s) found in pull request changed files. :white_check_mark: No issues found!

trogers-twilio commented 3 months ago

@dremin Love this idea! Just have a couple questions.

  1. To avoid the race condition of attempting to check if a feature is enabled before all features are enabled, is it suggested the developer use the "pluginsInitialized" event to know when it's safe to check if a feature is enabled?
  2. Should the template docs be updated to highlight this new method for checking if a feature is enabled, including how to ensure they avoid the potential race condition?
dremin commented 3 months ago
  1. To avoid the race condition of attempting to check if a feature is enabled before all features are enabled, is it suggested the developer use the "pluginsInitialized" event to know when it's safe to check if a feature is enabled?

The array is valid as soon as the first deferred hook begins to execute. This means we can actually access the array from some hooks before pluginsInitialized, as that event doesn't fire until after deferred hooks execute. That said, this is sensitive to the access time-- for example, if it is read from a module-level constant, then the access would actually happen when it is pulled in by the feature's index.ts, which would be too early. But if it is read from within the hook function, it isn't accessed until the hook executes, so that would be fine.

As you can tell, this could be easy to mis-use. I suppose we could address this somewhat by adding a console warning/error when it is accessed too early?

  1. Should the template docs be updated to highlight this new method for checking if a feature is enabled, including how to ensure they avoid the potential race condition?

I'm inclined to not document its existence due to the complexity outlined above...

trogers-twilio commented 3 months ago
  1. To avoid the race condition of attempting to check if a feature is enabled before all features are enabled, is it suggested the developer use the "pluginsInitialized" event to know when it's safe to check if a feature is enabled?

The array is valid as soon as the first deferred hook begins to execute. This means we can actually access the array from some hooks before pluginsInitialized, as that event doesn't fire until after deferred hooks execute. That said, this is sensitive to the access time-- for example, if it is read from a module-level constant, then the access would actually happen when it is pulled in by the feature's index.ts, which would be too early. But if it is read from within the hook function, it isn't accessed until the hook executes, so that would be fine.

As you can tell, this could be easy to mis-use. I suppose we could address this somewhat by adding a console warning/error when it is accessed too early?

  1. Should the template docs be updated to highlight this new method for checking if a feature is enabled, including how to ensure they avoid the potential race condition?

I'm inclined to not document its existence due to the complexity outlined above...

Considering the need to check if another feature is enabled is a fairly common use case, I'd prefer we provide some documentation on how to safely do so, and this new approach is better than checking Flex config directly for all the reasons you mentioned in the PR. Users of the template will inevitable use this new approach as they look to existing features for examples of how to do it. I'd rather we make the timing concern explicit in our docs versus leaving it open to developers only finding it during testing.

If there is a reliable way of also logging a warning / error when it's read too early, that would be a great improvement. Could drastically reduce troubleshooting time if a developer encounters it.

dremin commented 3 months ago

Considering the need to check if another feature is enabled is a fairly common use case, I'd prefer we provide some documentation on how to safely do so, and this new approach is better than checking Flex config directly for all the reasons you mentioned in the PR. Users of the template will inevitable use this new approach as they look to existing features for examples of how to do it. I'd rather we make the timing concern explicit in our docs versus leaving it open to developers only finding it during testing.

If there is a reliable way of also logging a warning / error when it's read too early, that would be a great improvement. Could drastically reduce troubleshooting time if a developer encounters it.

Fair enough! I have an idea on how to do this, and we can mention the log as part of the doc for the util so that developers are aware of what to expect.

dremin commented 3 months ago

@trogers-twilio I've added the warning as well as docs, let me know what you think!