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

Modern ES2015+ syntax and ESModules #228

Closed

Conversation

BasBastian
Copy link

@BasBastian BasBastian commented Mar 6, 2022

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
Copy link
Author

BasBastian commented Mar 6, 2022

@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.

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

--

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
@nigelmegitt
Copy link
Contributor

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

@BasBastian
Copy link
Author

BasBastian commented Mar 7, 2022

Motivation behind the change @nigelmegitt

  1. Solving issues Upgrade to use es6 language features? #214 and Support es6 syntax features part 1 #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

@nigelmegitt
Copy link
Contributor

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?

@BasBastian
Copy link
Author

BasBastian commented Mar 7, 2022

Sorry, I meant #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

@palemieux
Copy link
Contributor

Replaced by #255

@palemieux palemieux closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants