-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix/#4751 b #4766
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
base: development
Are you sure you want to change the base?
Fix/#4751 b #4766
Conversation
I have tried this fix but is not working! I got no typescript errors but dashjs from default import is undefined. So import dashjs from 'dashjs'; yields to undefined. I have looked at index.js and there is export default dashjs;. I suspect that dashjs is window.dashjs. I'm not sure why this is not working, because I think that this export should correcly export window.dashjs variable. export { b as Constants, y as Debug, E as MediaPlayer, _ as MediaPlayerFactory, T as MetricsReporting, v as Protection, w as default, S as supportsMediaSource } no default export??? So it looks like named export is correctly set so I changed index.d.ts: //export default dashjs;
//export as namespace dashjs;
export function MediaPlayer(): dashjs.MediaPlayerFactory;
export interface MediaPlayerFactory {
create(): dashjs.MediaPlayerClass;
}
export namespace MediaPlayer {
export const events: dashjs.MediaPlayerEvents;
export const errors: dashjs.MediaPlayerErrors;
}
declare namespace dashjs {
..} and now I'm able to import like: import { MediaPlayer } from 'dashjs'; and it works. Sorry, but for more investigation currently I don't have time. P.S. is it possible then when you bundle minification files, export default is omitted because there is no local variable dashjs? |
Can you supply a minimum sample app or does it match with what we provide already here: https://github.com/Dash-Industry-Forum/dash.js/tree/development/samples/modules ? Except for the way the import is implemented:
|
Hi, git clone https://github.com/edvinv/dashjs-angular.git
npm ci
npm start and you should get typescript error:
To get errors from libs you must have "skipLibCheck" set to false in typescript config. If dashjs only support named exports and global window.dashjs then you should change like this: export as namespace dashjs;
declare module 'dashjs' {
// types go here
} This works and you are able to import like: import {MediaPlayer} from 'dashjs'
// or
import * as dashjs from 'dashjs' Default imports are, at least what I tested not working. Anyway, IMO it is not good to mix both named and default. So you should also check what is the purpose of export default dashjs; in index.js. I also noticed, that you are using Buffer type in d.ts. This is node type and it's not good to use it in frontend, because you need node types and you will get global types pollution. |
'declare module' did not work for me; however removing the declare-block entirely like so: export as namespace dashjs;
export namespace MediaPlayer{
...
}
// Same for rest of types has worked. Afterwards the imports can simply be done as described. If a default export needs to be kept, accessing the types becomes more covoluted; eg. Mediaplayer and Debug would have to be accessed like so: import MediaPlayer from 'dashjs';
import dashjs from 'dashjs';
declare const Debug: dashjs.Debug;
MediaPlayer.MediaPlayer().create();
Debug.something(); so the best case seems to be to go with only a namespace export and named exports. As for the node.js Buffer type, I have replaced it with getBuffer(): SourceBuffer | TextSourceBuffer; SourceBuffer being the MediaSourceExtension source buffer type MDN reference from typescript/lib, and TextSourceBuffer being a dashjs type also described in index.d.ts. I will push the changes to this branch if they are acceptable. |
…nflict with ECMA script; fixed return types of SourceBuffer getters/setters
I have merged the most recent changes of dash.js development, fixed merge conflicts and pushed the fix to this branch for now. |
Fixes #4751 (comment).
@edvinv: Can you confirm this fixes your issue. When running
tsc
I didn't see the error.