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

⚡️(frontend) lib-video live transpiling #2150

Merged
merged 9 commits into from
Mar 29, 2023

Conversation

AntoLC
Copy link
Contributor

@AntoLC AntoLC commented Mar 24, 2023

Purpose

Only for lib_video for the moment.

Improve development process by not having to rebuild our packages after a change.
We will listen our packages from our applications and transpile automatically when a change happen.

Proposal

  • package marsha-config to mutualize and organize our configs (tsconfig , packages, config...)
  • package eslint-config-marsha to manage our linter
  • use Craco to customize the create-react-app configuration without ejecting the app
  • adapt lib_video with the new setup
  • remove rollup from lib-video
  • plug lib_video to standalone
  • plug lib_video to LTI

@AntoLC AntoLC added improvement feature dependencies Pull requests that update a dependency file Frontend labels Mar 24, 2023
@AntoLC AntoLC self-assigned this Mar 24, 2023
@AntoLC AntoLC force-pushed the feature/anthony/transpile-on-the-fly branch 10 times, most recently from fbc3ddf to d30e4c9 Compare March 28, 2023 13:28
@AntoLC AntoLC force-pushed the feature/anthony/transpile-on-the-fly branch 2 times, most recently from dad89f7 to fb4f252 Compare March 28, 2023 14:28
@AntoLC AntoLC marked this pull request as ready for review March 28, 2023 15:19
Copy link
Contributor

@kernicPanel kernicPanel left a comment

Choose a reason for hiding this comment

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

I didn't try to run it yet, but seems great !

Just a comment about filenames in src/frontend/packages/marsha-config/tsconfig/
Maybe we could use those:

  • app.json -> tsc_apps.json
  • base.json -> tsc_base.json
  • react-library.json -> tsc_packages.json

With something like that, we would be closer to our directory names.
WDYT ?

@AntoLC AntoLC force-pushed the feature/anthony/transpile-on-the-fly branch 2 times, most recently from c8d0372 to f22f0ca Compare March 29, 2023 08:48
@AntoLC
Copy link
Contributor Author

AntoLC commented Mar 29, 2023

Some fixup about what we said during the daily (renaming tsconfig files, lint order between eslint and tsc --noEmit).

Comment on lines 18 to 19
"../../../packages/lib_video/src/*",
"../../packages/lib_video/src/*"
Copy link
Contributor Author

@AntoLC AntoLC Mar 29, 2023

Choose a reason for hiding this comment

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

I forgot to talk about that, we have to double our path in this way, because we have 2 perspectives the standalone one and the LTI one, in the lti tsconfig the baseUrl is at the root ".", while in the standalone the baseUrl is at ./src, so the relative import are different depend the app.

moduleNameMapper,
} = require('marsha-config');

module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain with a comment what you are modifying here from the original webpack config in the create-react-app package ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"eject": "react-scripts eject",
"lint": "eslint src/**/*.{ts,tsx}",
"lint": "eslint src/**/*.{ts,tsx} && tsc --noEmit",
"lint:fix": "yarn run lint -- --fix",
Copy link
Member

Choose a reason for hiding this comment

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

This should not be working anymore I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

The lint command is ok, but not lint:fix anymore, because it resolves as eslint src/**/*.{ts,tsx} && tsc --noEmit -- --fix
Besides, tsc seems quicker than eslint, so I think we should keep it in the same order than before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I rolled back so: 340a31b

"postbuild": "./postbuild.sh",
"test": "cross-env CI=true TZ=UTC react-scripts test",
"test": "cross-env CI=true TZ=UTC craco test",
"eject": "react-scripts eject",
Copy link
Member

Choose a reason for hiding this comment

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

Do we keep this command ? We know it exists and we are working on how to not eject the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,21 @@
{
"name": "eslint-config-marsha",
"version": "0.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

we should use the same version used in all other packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"lint": "eslint src/**/*.{ts,tsx}",
"lint:fix": "yarn run lint -- --fix",
"lint": "eslint src/**/*.{ts,tsx} && tsc --noEmit",
"lint:fix": "yarn run lint --fix",
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this should not be working anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@kernicPanel kernicPanel left a comment

Choose a reason for hiding this comment

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

@AntoLC AntoLC requested a review from lunika March 29, 2023 10:49
This package is used to share config files between our
frontend apps and packages.
tsconfig:
  - base.json: base tsconfig file used by all frontend packages
  - app.json: tsconfig file used by our frontend apps
  - react-library.json: tsconfig file used by our frontend packages

You can still overide your configuration inside the
tsconfig.json file in your app directory.
Craco is a tool that allows to customize the create-react-app
configuration without ejecting the app.
We will be able to listen our different packages to tranpile on the fly.
Plug the new tsconfig to lib_video, to externalize and mutualize
 the typescript configuration.
- add lib_video package in the standalone site
- plug it with craco and webpack to have the automatic transpilation,
  a change in the lib_video package will be automatically transpiled
   and available in the standalone site.
By including lib-video in the standalone webpack, we got relative
imports problems coming from lib-video, it is because the
relative import are from the standalone perspective.
This commit fixes this issue by using the `@lib-video` alias.
- genere routes packages for webpack
- genere aliases for webpack
- genere moduleNameMapper for jest
@AntoLC AntoLC force-pushed the feature/anthony/transpile-on-the-fly branch from 720185d to 39bfcf4 Compare March 29, 2023 13:41
@AntoLC AntoLC enabled auto-merge (rebase) March 29, 2023 13:48
We don't need to build lib_video anymore, it is transpile during
the apps build.
- review dependencies packages about lib-video
- 🔥  all build relative dependencies from lib_video
- 🔥 rollup.config.mjs from lib_video
- adapt CI config and frontend package
To share our eslint configuration with other projects, we created
eslint-config-marsha package.
This package contains our eslint configuration and all the dependencies
needed to run it.
- add lib_video package in the LTI with live transpilation
- update the tsconfig.json to use the one in marsha-config
- update the webpack.config.js to add the alias of lib_video
- update the app.json in marsha-config to fit LTI hierarchy,
  LTI doesn't use a src folder
- remove not used libs
@AntoLC AntoLC force-pushed the feature/anthony/transpile-on-the-fly branch from 39bfcf4 to 1f5b033 Compare March 29, 2023 14:07
@AntoLC AntoLC merged commit 062254d into master Mar 29, 2023
@AntoLC AntoLC deleted the feature/anthony/transpile-on-the-fly branch March 29, 2023 15:33
lunika added a commit that referenced this pull request Apr 19, 2023
Added

- standalone website:
  - Integrate VOD dashboard (#2086)
  - List the lives in the contents section (#2104)
  - Live session model
  - Livesession backend rewrite
  - Add sentry
  - Create a live from the website (#2134)
  - Integrate webinar dashboard (#2135)
- Add a License Manager widget for LTI VOD view
- Add a title to the classroom file dropzone
- Add can_edit property on a serialized video
- Add an attribute to consumer site to deactivate resources in LTI select
- live transpilation on lib-video (#2150)
- live transpilation on lib-classroom (#2157)
- live transpilation on lib-markdown (#2160)
- live transpilation on lib-components (#2161)
- live transpilation on lib-tests (#2163)
- live transpilation on lib-common (#2164)
- Add a widget provider for the classroom creation form
- Allow delete playlist resources
  - FileDepository
  - Classroom
  - Document
  - Markdown
- Allow delete playlist
- add routes related URL:
  - thumbnails
  - timed_text_track
  - shared_live_media
- Add classroom widgets :
  - InfoBar
  - Description
  - Scheduling
  - Invite links
  - Support sharing
  - Recordings
- Add classroom invite link for an instructor
- Add a "Tools & Applications" widget for classrooms

Changed

- Update live sessions API to use nested video ID route
- Move generic widget components from lib-video to lib-components
- Make video dashboard collapsed by default
- improve the dropdown languages positionning in the dashboard (#2138)
- Make video dashboard visible by default, and collapsed when using the
  Moodle atto plugin
- Update live_session api to use mixin to prevent url crafting
- standalone website:
  - put the creating ressource form submit button disabled when the
    form is invalid (#2175)

Fixed

- redirect to error page when VOD is deleted
- Manage disconnection when multiple tabs were open on standalone site
- synchronisation between pages for the VOD description widget
- tooltip position on the website context dashboard (#2136)
- thumbnail not reset correctly on the video player (#2137)
- display title / description when a classroom is not scheduled and not started
- correctly fetch transcript content in TranscriptReader
- remove unused 'initiate-live' permissions
- increase debounce time in classroom description widget
- remove id3 tags upload when live channel is not ready
- add an invitation link for moderators in a launched classroom if available
lunika added a commit that referenced this pull request Apr 19, 2023
Added

- standalone website:
  - Integrate VOD dashboard (#2086)
  - List the lives in the contents section (#2104)
  - Live session model
  - Livesession backend rewrite
  - Add sentry
  - Create a live from the website (#2134)
  - Integrate webinar dashboard (#2135)
- Add a License Manager widget for LTI VOD view
- Add a title to the classroom file dropzone
- Add can_edit property on a serialized video
- Add an attribute to consumer site to deactivate resources in LTI select
- live transpilation on lib-video (#2150)
- live transpilation on lib-classroom (#2157)
- live transpilation on lib-markdown (#2160)
- live transpilation on lib-components (#2161)
- live transpilation on lib-tests (#2163)
- live transpilation on lib-common (#2164)
- Add a widget provider for the classroom creation form
- Allow delete playlist resources
  - FileDepository
  - Classroom
  - Document
  - Markdown
- Allow delete playlist
- add routes related URL:
  - thumbnails
  - timed_text_track
  - shared_live_media
- Add classroom widgets :
  - InfoBar
  - Description
  - Scheduling
  - Invite links
  - Support sharing
  - Recordings
- Add classroom invite link for an instructor
- Add a "Tools & Applications" widget for classrooms

Changed

- Update live sessions API to use nested video ID route
- Move generic widget components from lib-video to lib-components
- Make video dashboard collapsed by default
- improve the dropdown languages positionning in the dashboard (#2138)
- Make video dashboard visible by default, and collapsed when using the
  Moodle atto plugin
- Update live_session api to use mixin to prevent url crafting
- standalone website:
  - put the creating ressource form submit button disabled when the
    form is invalid (#2175)

Fixed

- redirect to error page when VOD is deleted
- Manage disconnection when multiple tabs were open on standalone site
- synchronisation between pages for the VOD description widget
- tooltip position on the website context dashboard (#2136)
- thumbnail not reset correctly on the video player (#2137)
- display title / description when a classroom is not scheduled and not started
- correctly fetch transcript content in TranscriptReader
- remove unused 'initiate-live' permissions
- increase debounce time in classroom description widget
- remove id3 tags upload when live channel is not ready
- add an invitation link for moderators in a launched classroom if available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical dependencies Pull requests that update a dependency file feature Frontend improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants