Skip to content

Commit

Permalink
fix(types): Add has|usingPlugin to typedef by adding stubs which are …
Browse files Browse the repository at this point in the history
…removed from builds (#8811)

## Description
tsc doesn't understand mixins and ignores jsdoc not followed by code.
The jsdoc for the plugin methods `usingPlugin()` and `hasPlugin()` in
Player are being ignored. To get them included in type outputs we need
to have otherwise unnecessary stubs codes, as we already have for `on()`
etc, which adds unnecessary, even if a small amount of, code to the
outputs.

## Specific Changes proposed
* Slight refactor of Player to include those stubs.
* Adds a rollup plugin to delete lines between certain comments, so
those stubs are deleted from the outputs.
* Applies those comments to the `on()` etc stubs in Component and the
new plugin stubs in Player.

Any code surrounded by these comments, and the comments themselves, is
deleted from the dist and test builds:

```js
/* start-delete-from-build */
  console.log('hi');
/* start-delete-from-build */
```

Compared to main, video.min.js is 53 bytes smaller.

## Requirements Checklist
- [ ] Feature implemented / Bug fixed
- [ ] If necessary, more likely in a feature request than a bug fix
- [ ] Change has been verified in an actual browser (Chrome, Firefox,
IE)
  - [ ] Unit Tests updated or fixed
  - [ ] Docs/guides updated
- [ ] Example created ([starter template on
JSBin](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0))
- [ ] Has no DOM changes which impact accessiblilty or trigger warnings
(e.g. Chrome issues tab)
  - [ ] Has no changes to JSDoc which cause `npm run docs:api` to error
- [ ] Reviewed by Two Core Contributors
  • Loading branch information
mister-ben authored Aug 12, 2024
1 parent 3380d33 commit 820ef38
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 28 deletions.
41 changes: 41 additions & 0 deletions build/rollup-exclude-lines.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Remove parts of files from outputs. Everything between a pair of `/* start-delete-from-build *\u002f`
* and `/* end-delete-from-build *\u002f` comments
*
* Based on https://github.com/se-panfilov/rollup-plugin-strip-code
*/

import { createFilter } from '@rollup/pluginutils';

const START_COMMENT = 'start-delete-from-build';
const END_COMMENT = 'end-delete-from-build';

/**
* Remove lines of code surrounded by comments
*
* @param {Object} [options] Options
* @param {string} [options.include] Files to inlcude
* @param {string} [options.exclude] Files to exclude
* @param {string} [options.startComment] Starting keywork, default start-delete-from-build
* @param {string} [options.endComment] Eding keywork, default end-delete-from-build
* @param {RegExp} [options.pattern] Custom regex
* @return void
*/
export default function excludeLines(options = {}) {
// assume that the myPlugin accepts options of `options.include` and `options.exclude`
const filter = createFilter(options.include, options.exclude);

return {
transform(code, id) {
if (!filter(id)) {
return;
}

const startComment = options.startComment || START_COMMENT;
const endComment = options.endComment || END_COMMENT;
const defaultPattern = new RegExp(`([\\t ]*\\/\\* ?${startComment} ?\\*\\/)[\\s\\S]*?(\\/\\* ?${endComment} ?\\*\\/[\\t ]*\\n?)`, 'g');

return code.replace(options.pattern || defaultPattern, '');
}
};
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
"@babel/preset-env": "^7.9.0",
"@rollup/plugin-image": "^3.0.2",
"@rollup/plugin-replace": "^2.4.1",
"@rollup/pluginutils": "^5.1.0",
"@types/node": "^18.8.3",
"access-sniff": "^3.2.0",
"autoprefixer": "^10.2.5",
Expand Down
25 changes: 25 additions & 0 deletions rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import image from '@rollup/plugin-image';
import istanbul from 'rollup-plugin-istanbul';
import externalGlobals from 'rollup-plugin-external-globals';
import svg from 'rollup-plugin-svg';
import excludeLines from './build/rollup-exclude-lines';

const excludeCoverage = [
'test/**',
Expand Down Expand Up @@ -143,6 +144,9 @@ export default cliargs => [
},
external: externals.browser,
plugins: [
excludeLines({
include: 'src/js/**'
}),
alias({
'video.js': path.resolve(__dirname, './src/js/video.js')
}),
Expand All @@ -169,6 +173,9 @@ export default cliargs => [
},
external: externals.browser,
plugins: [
excludeLines({
include: 'src/js/**'
}),
alias({
'video.js': path.resolve(__dirname, './src/js/video.js')
}),
Expand All @@ -193,6 +200,9 @@ export default cliargs => [
},
external: externals.test,
plugins: [
excludeLines({
include: 'src/js/**'
}),
multiEntry({exports: false}),
alias({
'video.js': path.resolve(__dirname, './src/js/video.js')
Expand Down Expand Up @@ -228,6 +238,9 @@ export default cliargs => [
],
external: externals.module,
plugins: [
excludeLines({
include: 'src/js/**'
}),
alias({
'video.js': path.resolve(__dirname, './src/js/video.js'),
'videojs-contrib-quality-levels': path.resolve(__dirname, './node_modules/videojs-contrib-quality-levels/dist/videojs-contrib-quality-levels.es.js'),
Expand Down Expand Up @@ -260,6 +273,9 @@ export default cliargs => [
external: externals.browser,
plugins: [
primedIgnore,
excludeLines({
include: 'src/js/**'
}),
alias({
'video.js': path.resolve(__dirname, './src/js/video.js')
}),
Expand Down Expand Up @@ -292,6 +308,9 @@ export default cliargs => [
],
external: externals.module,
plugins: [
excludeLines({
include: 'src/js/**'
}),
json(),
primedBabel,
svg(),
Expand All @@ -313,6 +332,9 @@ export default cliargs => [
external: externals.browser,
plugins: [
primedResolve,
excludeLines({
include: 'src/js/**'
}),
json(),
primedExternalGlobals,
primedCjs,
Expand All @@ -337,6 +359,9 @@ export default cliargs => [
plugins: [
primedIgnore,
primedResolve,
excludeLines({
include: 'src/js/**'
}),
json(),
primedExternalGlobals,
primedCjs,
Expand Down
10 changes: 10 additions & 0 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ class Component {
* @param {Function} fn
* The function to call with `EventTarget`s
*/
/* start-delete-from-build */
on(type, fn) {}
/* end-delete-from-build */

/**
* Removes an `event listener` for a specific event from an instance of `EventTarget`.
Expand All @@ -164,7 +166,9 @@ class Component {
* @param {Function} [fn]
* The function to remove. If not specified, all listeners managed by Video.js will be removed.
*/
/* start-delete-from-build */
off(type, fn) {}
/* end-delete-from-build */

/**
* This function will add an `event listener` that gets triggered only once. After the
Expand All @@ -177,7 +181,9 @@ class Component {
* @param {Function} fn
* The function to be called once for each event name.
*/
/* start-delete-from-build */
one(type, fn) {}
/* end-delete-from-build */

/**
* This function will add an `event listener` that gets triggered only once and is
Expand All @@ -191,7 +197,9 @@ class Component {
* @param {Function} fn
* The function to be called once for each event name.
*/
/* start-delete-from-build */
any(type, fn) {}
/* end-delete-from-build */

/**
* This function causes an event to happen. This will then cause any `event listeners`
Expand All @@ -212,7 +220,9 @@ class Component {
* @param {Object} [hash]
* Optionally extra argument to pass through to an event listener
*/
/* start-delete-from-build */
trigger(event, hash) {}
/* end-delete-from-build */

/**
* Dispose of the `Component` and all child components.
Expand Down
66 changes: 38 additions & 28 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -5323,6 +5323,44 @@ class Player extends Component {
*/
this.trigger('playbackrateschange');
}

/**
* Reports whether or not a player has a plugin available.
*
* This does not report whether or not the plugin has ever been initialized
* on this player. For that, [usingPlugin]{@link Player#usingPlugin}.
*
* @method hasPlugin
* @param {string} name
* The name of a plugin.
*
* @return {boolean}
* Whether or not this player has the requested plugin available.
*/
/* start-delete-from-build */
hasPlugin(name) {
return false;
}
/* end-delete-from-build */

/**
* Reports whether or not a player is using a plugin by name.
*
* For basic plugins, this only reports whether the plugin has _ever_ been
* initialized on this player.
*
* @method Player#usingPlugin
* @param {string} name
* The name of a plugin.
*
* @return {boolean}
* Whether or not this player is using the requested plugin.
*/
/* start-delete-from-build */
usingPlugin(name) {
return false;
}
/* end-delete-from-build */
}

/**
Expand Down Expand Up @@ -5525,33 +5563,5 @@ TECH_EVENTS_RETRIGGER.forEach(function(event) {
* @type {Event}
*/

/**
* Reports whether or not a player has a plugin available.
*
* This does not report whether or not the plugin has ever been initialized
* on this player. For that, [usingPlugin]{@link Player#usingPlugin}.
*
* @method Player#hasPlugin
* @param {string} name
* The name of a plugin.
*
* @return {boolean}
* Whether or not this player has the requested plugin available.
*/

/**
* Reports whether or not a player is using a plugin by name.
*
* For basic plugins, this only reports whether the plugin has _ever_ been
* initialized on this player.
*
* @method Player#usingPlugin
* @param {string} name
* The name of a plugin.
*
* @return {boolean}
* Whether or not this player is using the requested plugin.
*/

Component.registerComponent('Player', Player);
export default Player;

0 comments on commit 820ef38

Please sign in to comment.