Migrate asciicinema component from hashicorp/ember-asciinema-player repo #3102
Conversation
Migrated from repo: https://github.com/hashicorp/ember-asciinema-player At commit: 09db104dd1db96348351a1cf50315cd00c3189dd
Styles imported from ember-asciinema-player are only importing styles from asciinema-player. See: https://github.com/hashicorp/ember-asciinema-player/blob/09db104dd1db96348351a1cf50315cd00c3189dd/app/styles/%40hashicorp/ember-asciinema-player.scss#L6
Rename class name to match the class naming conventions for generated components
The player property doesn't need the @Tracked because its not being used by ember to render. It is not rendered itself and changes to the property do not impact what ember renders. The player property only needs to track the previous player reference during teardown
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| {{did-insert this.initializePlayer}} | ||
| {{will-destroy this.destroyPlayer}} |
There was a problem hiding this comment.
ember template lint warns against using these
| */ | ||
| @action | ||
| destroyPlayer() { | ||
| later(() => this.dispose(), 250); |
There was a problem hiding this comment.
ember eslint warns against using runloop functions
| assert.expect(1); | ||
|
|
||
| const asciicast = await fetch('/example.cast'); | ||
| const asciicast = await fetch('/session.cast'); |
There was a problem hiding this comment.
In our app we have the same cast file as this test was using in the repo, it's just under a different filename
| }} | ||
|
|
||
| <div ...attributes {{this.initializePlayer}}></div> | ||
| <div ...attributes {{this.initializePlayer data=@data}}></div> |
There was a problem hiding this comment.
It seemed better to bring in the @data through the element modifier args than reference this.args in the modifier logic although in both cases this.args.* usage should be tracked, this way is a bit more explicit as a dependency input to the modifier function
| * @type {?AsciinemaPlayer} | ||
| */ | ||
| @tracked player = null; | ||
| player = null; |
There was a problem hiding this comment.
I set up a modified test where the data arg was set, and then 4 seconds later set with a new cast input. This change was required otherwise ember detected that this.player is being read and set in the same autotracking routing which it errors could cause infinite render loops. This is because the modifier dispose function sets a this.player = null and then a new this.player is being set.
In reality though this can all be avoided because player doesn't need to be @tracked. Render does not need to know when it changes to update something it is responsible for rendering. Everything within the div passed to AsciinemaPlayer is managed by that library
| "overrides": { | ||
| "nomnom>underscore": "^1.12.1", | ||
| "@hashicorp/design-system-components>ember-stargate": "^0.6.0", | ||
| "@hashicorp/ember-asciinema-player>asciinema-player": "3.4.0", |
There was a problem hiding this comment.
We previously had to lock in on this version because we relied on specific internal DOM structure and classes that changed after 3.4.0. With this PR we can remove this but still need to ensure that our dependency brought in within admin UI is 3.4.0. When we upgrade to a newer version we'll have make sure our additional css styles still apply
| }, | ||
| "dependencies": { | ||
| "@babel/runtime": "7.27.6", | ||
| "asciinema-player": "3.4.0", |
There was a problem hiding this comment.
1 participant
Description
Migrates the component from hashicorp/ember-asciinema-player into admin ui. We use it one place within this app. It's easier to maintain within this app and also allows us to make changes as needed.
To see how the existing files from the repo were migrated it's best to go commit-by-commit on this PR.
How to Test
Go to the session recordings page and ensure that our sample cast is still playable.
Checklist
a11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.