Repository: chanind/hanzi-writer (ts)

Issue:
Constructor does real work
The current constructor of HanziWriter is calling ``setCharacter()``. Which is too soon in my code, and don't let me really manage when ``setCharacter()`` gets called. This is a minor issue, but is a little annoying and goes against some (debatable) design principles : a constructor should not do any real work (only setting up inner variables).

I'm willing to make a PR on that, taking into account the fact that you need to keep backwards compatibility.

To elaborate why it is better to avoid any complex behavior in constructors, here are a few discussion about that:
    http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/
    https://softwareengineering.stackexchange.com/questions/224063
    https://softwareengineering.stackexchange.com/questions/159193
    https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c41-a-constructor-should-create-a-fully-initialized-object
    https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-factory

As a fix, I would suggest to make it optional (default would still be to call ``setCharacter()``, but this should be removed in next major release), and provide a factory that would create the widget and load a character ``HanziWriter.Create()`` to still offer the use case of setting up and loading a character in one go.

I'm totally aware that it's kind'a matter of taste here, and I will very happily and kindly understand any dismissal of this proposal.

Pull request:
Simplifying constructor, but keeping backwards compatibility
As discussed in #55, this PR simplifies the behavior of `new HanziWriter()` while maintaining backwards compatibility. This PR thus allows setup via the following methods:

1. `writer = new HanziWriter('target', '人', options)` original behavior. This will now `console.warn` that this is deprecated.
2. `writer = HanziWriter.create('target', '人', options)` Helper which mimics old constructor behavior
3. `writer = new HanziWriter('target', options); writer.setCharacter('人');`
4. `writer = new HanziWriter(); writer.init('target', options); writer.setCharacter('人');`

I'm leaning towards removing option `[4]` above, since it's a bit awkward with needing to call `writer.init('target', options)` after `new HanziWriter()` but before `setCharacter()`, which seems a bit verbose. But it's maybe also the most technically correct as the constructor does literally nothing then? thoughts @vaab and @Mikurox?

This is a large enough change that it probably makes sense to put it into the `1.0` release

closes #55 


Patch:
diff --git a/src/HanziWriter.js b/src/HanziWriter.js
index 909260a9..35181412 100644
--- a/src/HanziWriter.js
+++ b/src/HanziWriter.js
@@ -55,16 +55,25 @@ const defaultOptions = {
   outlineWidth: 2,
 };
 
-function HanziWriter(element, character, options = {}) {
-  this._canvas = svg.Canvas.init(element, options.width, options.height);
-  if (this._canvas.svg.createSVGPoint) {
-    this._pt = this._canvas.svg.createSVGPoint();
+function HanziWriter(...args) {
+  if (args.length > 0) {
+    let character;
+    let options = {};
+    const element = args[0];
+    if (args.length > 1) {
+      if (typeof args[1] === 'string') {
+        console.warn('Using new HanziWriter() to set a character is deprecated. Use HanziWriter.create() instead');
+        character = args[1];
+        options = args[2] || {};
+      } else {
+        options = args[1];
+      }
+    }
+    this._init(element, options);
+    if (character) {
+      this.setCharacter(character);
+    }
   }
-  this._options = this._assignOptions(options);
-  this._loadingManager = new LoadingManager(this._options);
-  this.setCharacter(character);
-  this._setupListeners();
-  this._quiz = null;
 }
 
 // ------ public API ------ //
@@ -172,6 +181,18 @@ HanziWriter.prototype.setCharacter = function(char) {
 
 // ------------- //
 
+HanziWriter.prototype._init = function(element, options) {
+  this._canvas = svg.Canvas.init(element, options.width, options.height);
+  if (this._canvas.svg.createSVGPoint) {
+    this._pt = this._canvas.svg.createSVGPoint();
+  }
+  this._options = this._assignOptions(options);
+  this._loadingManager = new LoadingManager(this._options);
+  this._setupListeners();
+  this._quiz = null;
+  return this;
+};
+
 HanziWriter.prototype._assignOptions = function(options) {
   const mergedOptions = assign({}, defaultOptions, options);
 
@@ -280,6 +301,12 @@ HanziWriter.prototype._getTouchPoint = function(evt) {
 
 // --- Static Public API --- //
 
+HanziWriter.create = (element, character, options = {}) => {
+  const writer = new HanziWriter(element, options);
+  writer.setCharacter(character);
+  return writer;
+};
+
 let lastLoadingManager = null;
 let lastLoadingOptions = null;
 


Test patch:
diff --git a/src/__tests__/HanziWriter-test.js b/src/__tests__/HanziWriter-test.js
index 72cc83fd..8fbd0afd 100644
--- a/src/__tests__/HanziWriter-test.js
+++ b/src/__tests__/HanziWriter-test.js
@@ -19,7 +19,27 @@ describe('HanziWriter', () => {
   });
 
   describe('constructor', () => {
-    it('loads data and builds an instance in a dom element', async () => {
+    it('can optionally run init() if element and options are passed in', async () => {
+      document.body.innerHTML = '<div id="target"></div>';
+
+      const writer = new HanziWriter('target', { charDataLoader });
+      await writer.setCharacter('人');
+
+      expect(document.querySelectorAll('#target svg').length).toBe(1);
+      const svg = document.querySelector('#target svg');
+      expect(svg.childNodes.length).toBe(2);
+      expect(svg.childNodes[0].nodeName).toBe('defs');
+      expect(svg.childNodes[1].nodeName).toBe('g');
+      // the characters are repeated 3 times - one for outline, character, and highlight
+      expect(svg.childNodes[1].childNodes.length).toBe(3);
+      svg.childNodes[1].childNodes.forEach(charNode => {
+        expect(charNode.nodeName).toBe('g');
+        // 2 strokes per character
+        expect(charNode.childNodes.length).toBe(2);
+      });
+    });
+
+    it('[deprecated] loads data and builds an instance in a dom element', async () => {
       document.body.innerHTML = '<div id="target"></div>';
 
       const writer = new HanziWriter('target', '人', { charDataLoader });
@@ -41,6 +61,29 @@ describe('HanziWriter', () => {
     });
   });
 
+  describe('HanziWriter.create', () => {
+    it('loads data and builds an instance in a dom element', async () => {
+      document.body.innerHTML = '<div id="target"></div>';
+
+      const writer = HanziWriter.create('target', '人', { charDataLoader });
+
+      await writer._withDataPromise;
+
+      expect(document.querySelectorAll('#target svg').length).toBe(1);
+      const svg = document.querySelector('#target svg');
+      expect(svg.childNodes.length).toBe(2);
+      expect(svg.childNodes[0].nodeName).toBe('defs');
+      expect(svg.childNodes[1].nodeName).toBe('g');
+      // the characters are repeated 3 times - one for outline, character, and highlight
+      expect(svg.childNodes[1].childNodes.length).toBe(3);
+      svg.childNodes[1].childNodes.forEach(charNode => {
+        expect(charNode.nodeName).toBe('g');
+        // 2 strokes per character
+        expect(charNode.childNodes.length).toBe(2);
+      });
+    });
+  });
+
   describe('data loading', () => {
     it('calls onLoadCharDataError if provided on loading failure', async () => {
       document.body.innerHTML = '<div id="target"></div>';