imscJS icon indicating copy to clipboard operation
imscJS copied to clipboard

Modern ES2015+ syntax and ESModules

Open BasBastian opened this issue 3 years ago • 5 comments

This is an effort towards making this a Typescript library

  • added Babel.js in the process (merged with browserify)
  • preserved the compilation process
  • replaced legacy JSHint with JSLint
  • ensured that tests passes in QUnit
  • extracted error reporting functions into a separate error.js file
  • converted files into ESModules (the compilation target is still UMD though)
  • preserved the way that sax is included in "imsc.all." build and excluded from "imsc." builds

Breaking change!! If imscDoc, imscHTML was ever available in browser apart from global imsc namespace, they will be no longer. That can be restored, but IMHO the price is too high compared to the tech debt that's left in code

I made sure that the unit-tests passes, but I am not entirely sure about renders. Please advise in this case to avoid breaking rendering of subtitles

--

Todos:

  • extract IMSC objects classes into separate file
  • move to the Typescript compiler (google-closure-compiler can still be used for minification)
  • extract proper TS interfaces
  • export types and extract to *.d.ts definition file
  • fix potentially buggy code (replacing string with object in array, etc.)
  • use ValueObject pattern (immutable IMSC classes with parameter injection via constructor, instead of "initFromNode")
  • cloning process improvement

BasBastian avatar Mar 06 '22 11:03 BasBastian

@palemieux How do you feel about this option? I checked the whole process with QUnit and used NodeJS@12 for running the bundling process.

I think it would be useful to improve the Code Quality of the library by moving it a bit toward Typescript (if not entirely change to that), by extracting code into smaller modules and removing potentially buggy code

Please also voice your concerns about removed function signatures, if necessary. I'd love to hear your opinion on that as well as on the whole change that I proposed

Also about the breaking change that was introduced.

BasBastian avatar Mar 06 '22 11:03 BasBastian

@BasBastian It would really help me if you could explain the motivation behind these changes. What problems are you seeking to solve?

nigelmegitt avatar Mar 07 '22 10:03 nigelmegitt

Motivation behind the change @nigelmegitt

  1. Solving issues #214 and #229 mentioned by other users
  2. Improve maintainability of source code (unless you consider current codebase maintainable)
  3. I'd love to see Typescript definitions exported and code moved to TS, but at the moment I consider this hard to proceed with
  4. I tried to rewrite it with Typescript and found few interesting issues that might result in unexpected bugs: feature/rewrite-1.1.3-with-typescript

I could write a single issue and duplicate requests from few years ago, but I preferred to take proactive approach and share this PR as a possible solution to those issues.

I am well aware that this is a large and complex change. If you have an instruction on how to improve the app and are looking forward to make the code up to date with nowadays standards, I still would like to hear your opinion on that

BasBastian avatar Mar 07 '22 11:03 BasBastian

Thanks for that. The lack of discussion on #214 suggests we haven't yet got a clear view of whether that's the agreed direction to take. It looks like #229 is a pull request not an issue, and was raised by yourself @BasBastian so I'm not sure if you meant to reference it?

nigelmegitt avatar Mar 07 '22 11:03 nigelmegitt

Sorry, I meant https://github.com/sandflow/imscJS/issues/215 , my mistake; if I should proceed differently and raise a discussion in an issue before, I would gladly accept it. I am just annoyed by missing types and proper structures in this projects, as we tinker with IMSC node types and would prefer to use clean typing system

BasBastian avatar Mar 07 '22 12:03 BasBastian

Replaced by #255

palemieux avatar May 01 '24 16:05 palemieux