-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add an hlsjsConfig query param to populate the demo editor
#7473
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
Conversation
0d30cd5 to
61f544e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The editor is intended for hls.js config options only - not options specific to the demo page application. It is important to keep this space specific to hls.js configuration so that developers do not confuse demo page functionality options like "autoRecoverError" with built-in config supported by the library.
It's great to see the hls.js config options added to the URL. Can this PR focus only on that?
When selecting a test option that includes additional config (ex, emeEnabled and widevine setup options), they should appear in the editor upon making the selection. These changes break that functionality currently. Admittedly, some functional test fields that are not hls.js config options leak into the editor because of this (ex: allowedBufferedRangesInSeekTest is not an hls.js config option). Removing the options specific to item selection when switching to another item has been challenging, while this solves that, it does it at the cost of hiding those options from view, which is not acceptable.
I prefer the editor to be used only for hls.js config options. As a JSON editor and not a JS editor, it has some shortcomings; there is no support for certain config value types (ex, functions, non-finite numbers). That may be a good thing as we do not want to deliver arbitrary JS functions in demo page urls.
|
Should I keep the two separate then? Keep demo-only configs in |
61f544e to
df84bf6
Compare
demoConfig value to populate the editorhlsjsConfig query param to populate the demo editor
robwalch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run npm run prettier && npm run sanity-check to fix minor formatting issues and check lint and build tasks (there are a couple minor formatting issues).
The interaction with the "Persist" option (STORAGE_KEYS.Editor_Persistence) is interesting. Maybe it should be disabled in permalinks or when the "hlsjsConfig" param is present so that the URL contents are used rather than have the store config applied over them.
df84bf6 to
77c98bd
Compare
|
Sorry about the formatting issue, I had run I have updated the behavior so it will ignore the local values if we loaded a config from the URL, which should avoid some confusion. |
This PR will...
Add a new the
hlsjsConfigquery param to populate the editor on load. It is added automatically to the permalink whenever a change to the editor is applied.Why is this Pull Request needed?
It makes it easier to share the demo page with the right config (and playback URL).
Are there any points in the code the reviewer needs to double check?
I have done some QA (e.g. with/without
hlsjsConfig, with/without peristance), but I don't know if I have covered all the edge cases.Resolves issues:
Checklist