Reduce Immutable.js

[azu]

Reduce usage of Immutable is meaningful.

Immutablity is good, but Immutable.js is hard.

• Immutable.js block worker thread support #37
  • Need to Serialize/Deserialize between immutable.js and JavaScript
• Immutable.js project status is unclear https://github.com/immutable-js/immutable-js/issues/1689
• Pure ECMAScript expressive power is grown

Immutable is important, but base class should not base on JavaScript framework like Immutable.js. Pure domain object model is better and use helper library is ok. For example, immutable-array-prototype help model to manupulate array as immutable.

[sohonisaurabh]

@azu - Are we going to follow any strategy like below:

1. Create mock classes for ImmutableJS. These will use vanilla JS objects but keep Immutable API intact.
2. Point all usages of ImmutableJS to mock classes
3. Delete ImmutableJS library
4. Remove ImmutableJS code smells. Something like foo.bar instead of foo.get('bar')

Also, my unit tests don't pass with below error:

Cannot find module '@honkit/markdown' from 'src/parsers.js

Any idea how to fix this?

[azu]

Also, my unit tests don't pass with below error:

Probably, you need to doyarn run build at first. Because, https://github.com/honkit/honkit/tree/master/packages/%40honkit/markdown is migrated to TypeScript by @mizchi. So, @honkit/markdown/lib/* is empty in local and require function try to load modules from lib/*.

yarn run build on root dir generates lib/* file. https://github.com/honkit/honkit/blob/master/CONTRIBUTING.md#building-honkit

[azu]

Are we going to follow any strategy like below:

Good point! It seems good. We can try this approach in some model as small steps.

Addtionaly, We might want to add "Migrate to TypeScript" as Step 0. #24 This migrateion may cause unexpected regression.

We can prepare integration tests parallely to avoid regresion #65

[azu]

The pain point of current implementation.

The current models class is based on immutable.js. However, It lose TypeScript pros.

Because it adds static method to Immutable.js object.

Example https://github.com/honkit/honkit/blob/master/packages/honkit/src/models/page.ts

import Immutable from "immutable";
import yaml from "js-yaml";
import File from "./file";
import { hashString } from "./hash";

const Page = Immutable.Record({
    file: File(),

    // Attributes extracted from the YAML header
    attributes: Immutable.Map(),

    // Content of the page
    content: String(),

    // Direction of the text
    dir: String("ltr"),
});

Page.prototype.getFile = function () {
    return this.get("file");
};

// ... snip ...
/**
 * Load a page for a file
 * @param {File} file
 * @param {string} content
 * @return {Page}
 */

// @ts-expect-error ts-migrate(2339) FIXME: Property 'loadFile' does not exist on type 'Class'... Remove this comment to see the full error message
Page.loadFile = function (file, content) {
    return new Page({
        file: file,
        content: content,
    });
};

// @ts-expect-error ts-migrate(2339) FIXME: Property 'fromJSON' does not exist on type 'Class'... Remove this comment to see the full error message
Page.fromJSON = function (json) {
    return new Page({
        file: new File(json.file),
        // Attributes extracted from the YAML header
        attributes: Immutable.Map(json.atributes),
        // Content of the page
        content: json.content,
        // Direction of the text
        dir: json.dir,
    });
};

// @ts-expect-error ts-migrate(2339) FIXME: Property 'toJSON' does not exist on type 'Class'.
Page.toJSON = function (page) {
    return page.toJS();
};
Page.prototype.hash = function () {
    return hashString(JSON.stringify(this.toJS()));
};

export default Page;

At first, we want to get correct typing for static method of Model class.

Image code

import Immutable from "immutable";
import yaml from "js-yaml";
import File from "./file";
import { hashString } from "./hash";

const Page = Immutable.Record({
    file: File(),

    // Attributes extracted from the YAML header
    attributes: Immutable.Map(),

    // Content of the page
    content: String(),

    // Direction of the text
    dir: String("ltr"),
});

Page.prototype.getFile = function () {
    return this.get("file");
};

/**
 * Create a page for a file
 * @param {File} file
 * @return {Page}
 */
export const createForFile = function (file) {
    return new Page({
        file: file,
    });
};

/**
 * Load a page for a file
 * @param {File} file
 * @param {string} content
 * @return {Page}
 */

export const loadFile = function (file, content) {
    return new Page({
        file: file,
        content: content,
    });
};

export const fromJSON = function (json) {
    return new Page({
        file: new File(json.file),
        // Attributes extracted from the YAML header
        attributes: Immutable.Map(json.atributes),
        // Content of the page
        content: json.content,
        // Direction of the text
        dir: json.dir,
    });
};
const toJSON = function (page) {
    return page.toJS();
};
const addStaticMethod = <T,P>(Class:T, methods: P): T & P => {
    /* add static method to class */
};
export default addStaticMethod(Page, {
    createForFile,
    loadFile,
    fromJSON,
    toJSON,
});

[azu]

• Immutable.js is essentially unmaintained · Issue #1689 · immutable-js/immutable-js
• immutable-js-oss/immutable-js: Immutable persistent data collections for Javascript which increase efficiency and simplicity.

[azu]

Semgrep help us to migration to class.

rules:
  - id: model
    languages:
      - typescript
    message: |
      use class
    pattern: "const $X = Immutable.Record(...);"
    fix-regex:
      regex: 'const (\w+) = ([\s\S]+);'
      replacement: 'class \1 extends \2{'
    severity: WARNING
  - id: instance-method
    languages:
      - typescript
    message: |
      prototype method to class instance methods
    patterns: 
      - pattern: "$X.prototype.$Y = function(...){ ... }" 
      - pattern: "$X.prototype.$Y = function $Y(...){ ... }"
      - pattern-not-inside: function (...){ ... }
    fix-regex:
      regex: '^(.*)\.prototype\.(.*) = function.*\('
      replacement: '\2('
    severity: WARNING
  - id: static-method
    languages:
      - typescript
    message: |
      static method to class statics methods
    patterns:
      - pattern: "$X.$Y = function(...){ ... }"
      - pattern: "$X.$Y = function $Y(...){ ... }"
      - pattern-not-inside: function (...){ ... }
    fix-regex:
      regex: '^(.*?)\.(.*?) = function.*\('
      replacement: 'static \2('
    severity: WARNING
  - id: export
    languages:
      - typescript
    message: |
       fix export
    pattern: "export default $X;"
    fix-regex:
      regex: 'export default'
      replacement: '}\nexport default'
    severity: WARNING

This rule migrates prototype-based to class-based implementation.

diff --git a/packages/honkit/src/models/book.ts b/packages/honkit/src/models/book.ts
index 0bbdf702..756171a7 100644
--- a/packages/honkit/src/models/book.ts
+++ b/packages/honkit/src/models/book.ts
@@ -9,7 +9,7 @@ import Glossary from "./glossary";
 import Languages from "./languages";
 import Ignore from "./ignore";

-const Book = Immutable.Record({
+class Book extends Immutable.Record({
     // Logger for outptu message

     // @ts-expect-error ts-migrate(2554) FIXME: Expected 2 arguments, but got 0.
@@ -33,337 +33,337 @@ const Book = Immutable.Record({

     // List of children, if multilingual (String -> Book)
     books: Immutable.OrderedMap(),
-});
-
-Book.prototype.getLogger = function () {
-    return this.get("logger");
-};
-
-Book.prototype.getFS = function () {
-    return this.get("fs");
-};
-
-Book.prototype.getIgnore = function () {
-    return this.get("ignore");
-};
-
-Book.prototype.getConfig = function () {
-    return this.get("config");
-};
-
-Book.prototype.getReadme = function () {
-    return this.get("readme");
-};
-
-Book.prototype.getSummary = function () {
-    return this.get("summary");
-};
-
-Book.prototype.getGlossary = function () {
-    return this.get("glossary");
-};
-
-Book.prototype.getLanguages = function () {
-    return this.get("languages");
-};
-
-Book.prototype.getBooks = function () {
-    return this.get("books");
-};
-
-Book.prototype.getLanguage = function () {
-    return this.get("language");
-};
-
-/**
- Return FS instance to access the content
-
- @return {FS}
- */
-Book.prototype.getContentFS = function () {
-    const fs = this.getFS();
-    const config = this.getConfig();
-    const rootFolder = config.getValue("root");
-
-    if (rootFolder) {
-        // @ts-expect-error ts-migrate(2339) FIXME: Property 'reduceScope' does not exist on type 'Cla... Remove this comment to see the full error message
-        return FS.reduceScope(fs, rootFolder);
-    }
-
-    return fs;
-};
-
-/**
- Return root of the book
-
- @return {String}
- */
-Book.prototype.getRoot = function () {
-    const fs = this.getFS();
-    return fs.getRoot();
-};
-
-/**
- Return root for content of the book
-
- @return {String}
- */
-Book.prototype.getContentRoot = function () {
-    const fs = this.getContentFS();
-    return fs.getRoot();
-};
-
-/**
- Check if a file is ignore (should not being parsed, etc)
-
- @param {String} ref
- @return {Page|undefined}
- */
-Book.prototype.isFileIgnored = function (filename) {
-    const ignore = this.getIgnore();
-    const language = this.getLanguage();
-
-    // Ignore is always relative to the root of the main book
-    if (language) {
-        filename = path.join(language, filename);
-    }
-
-    return ignore.isFileIgnored(filename);
-};
-
-/**
- Check if a content file is ignore (should not being parsed, etc)
-
- @param {String} ref
- @return {Page|undefined}
- */
-Book.prototype.isContentFileIgnored = function (filename) {
-    const config = this.getConfig();
-    const rootFolder = config.getValue("root");
-
-    if (rootFolder) {
-        filename = path.join(rootFolder, filename);
-    }
-
-    return this.isFileIgnored(filename);
-};
-
-/**
- Return a page from a book by its path
-
- @param {String} ref
- @return {Page|undefined}
- */
-Book.prototype.getPage = function (ref) {
-    return this.getPages().get(ref);
-};
-
-/**
- Is this book the parent of language's books
-
- @return {Boolean}
- */
-Book.prototype.isMultilingual = function () {
-    return this.getLanguages().getCount() > 0;
-};
-
-/**
- Return true if book is associated to a language
-
- @return {Boolean}
- */
-Book.prototype.isLanguageBook = function () {
-    return Boolean(this.getLanguage());
-};
-
-/**
- Return a languages book
-
- @param {String} language
- @return {Book}
- */
-Book.prototype.getLanguageBook = function (language) {
-    const books = this.getBooks();
-    return books.get(language);
-};
-
-/**
- Add a new language book
-
- @param {String} language
- @param {Book} book
- @return {Book}
- */
-Book.prototype.addLanguageBook = function (language, book) {
-    let books = this.getBooks();
-    books = books.set(language, book);
-
-    return this.set("books", books);
-};
-
-/**
- Set the summary for this book
-
- @param {Summary}
- @return {Book}
- */
-Book.prototype.setSummary = function (summary) {
-    return this.set("summary", summary);
-};
-
-/**
- Set the readme for this book
-
- @param {Readme}
- @return {Book}
- */
-Book.prototype.setReadme = function (readme) {
-    return this.set("readme", readme);
-};
-
-/**
- Set the configuration for this book
-
- @param {Config}
- @return {Book}
- */
-Book.prototype.setConfig = function (config) {
-    return this.set("config", config);
-};
-
-/**
- Set the ignore instance for this book
-
- @param {Ignore}
- @return {Book}
- */
-Book.prototype.setIgnore = function (ignore) {
-    return this.set("ignore", ignore);
-};
-
-/**
- Change log level
-
- @param {String} level
- @return {Book}
- */
-Book.prototype.setLogLevel = function (level) {
-    this.getLogger().setLevel(level);
-    return this;
-};
-
-/**
- Create a book using a filesystem
-
- @param {FS} fs
- @return {Book}
- */
-
-// @ts-expect-error ts-migrate(2339) FIXME: Property 'createForFS' does not exist on type 'Cla... Remove this comment to see the full error message
-Book.createForFS = function createForFS(fs) {
-    return new Book({
-        fs: fs,
-    });
-};
-
-/**
- Infers the default extension for files
- @return {String}
- */
-Book.prototype.getDefaultExt = function () {
-    // Inferring sources
-    const clues = [this.getReadme(), this.getSummary(), this.getGlossary()];
-
-    // List their extensions
-    const exts = clues.map((clue) => {
-        const file = clue.getFile();
-        if (file.exists()) {
-            return file.getParser().getExtensions().first();
+}) {
+    getLogger = function () {
+        return this.get("logger");
+    };
+
+    getFS = function () {
+        return this.get("fs");
+    };
+
+    getIgnore = function () {
+        return this.get("ignore");
+    };
+
+    getConfig = function () {
+        return this.get("config");
+    };
+
+    getReadme = function () {
+        return this.get("readme");
+    };
+
+    getSummary = function () {
+        return this.get("summary");
+    };
+
+    getGlossary = function () {
+        return this.get("glossary");
+    };
+
+    getLanguages = function () {
+        return this.get("languages");
+    };
+
+    getBooks = function () {
+        return this.get("books");
+    };
+
+    getLanguage = function () {
+        return this.get("language");
+    };
+
+    /**
+     Return FS instance to access the content
+
+     @return {FS}
+     */
+    getContentFS = function () {
+        const fs = this.getFS();
+        const config = this.getConfig();
+        const rootFolder = config.getValue("root");
+
+        if (rootFolder) {
+            // @ts-expect-error ts-migrate(2339) FIXME: Property 'reduceScope' does not exist on type 'Cla... Remove this comment to see the full error message
+            return FS.reduceScope(fs, rootFolder);
+        }
+
+        return fs;
+    };
+
+    /**
+     Return root of the book
+
+     @return {String}
+     */
+    getRoot = function () {
+        const fs = this.getFS();
+        return fs.getRoot();
+    };
+
+    /**
+     Return root for content of the book
+
+     @return {String}
+     */
+    getContentRoot = function () {
+        const fs = this.getContentFS();
+        return fs.getRoot();
+    };
+
+    /**
+     Check if a file is ignore (should not being parsed, etc)
+
+     @param {String} ref
+     @return {Page|undefined}
+     */
+    isFileIgnored = function (filename) {
+        const ignore = this.getIgnore();
+        const language = this.getLanguage();
+
+        // Ignore is always relative to the root of the main book
+        if (language) {
+            filename = path.join(language, filename);
+        }
+
+        return ignore.isFileIgnored(filename);
+    };
+
+    /**
+     Check if a content file is ignore (should not being parsed, etc)
+
+     @param {String} ref
+     @return {Page|undefined}
+     */
+    isContentFileIgnored = function (filename) {
+        const config = this.getConfig();
+        const rootFolder = config.getValue("root");
+
+        if (rootFolder) {
+            filename = path.join(rootFolder, filename);
+        }
+
+        return this.isFileIgnored(filename);
+    };
+
+    /**
+     Return a page from a book by its path
+
+     @param {String} ref
+     @return {Page|undefined}
+     */
+    getPage = function (ref) {
+        return this.getPages().get(ref);
+    };
+
+    /**
+     Is this book the parent of language's books
+
+     @return {Boolean}
+     */
+    isMultilingual = function () {
+        return this.getLanguages().getCount() > 0;
+    };
+
+    /**
+     Return true if book is associated to a language
+
+     @return {Boolean}
+     */
+    isLanguageBook = function () {
+        return Boolean(this.getLanguage());
+    };
+
+    /**
+     Return a languages book
+
+     @param {String} language
+     @return {Book}
+     */
+    getLanguageBook = function (language) {
+        const books = this.getBooks();
+        return books.get(language);
+    };
+
+    /**
+     Add a new language book
+
+     @param {String} language
+     @param {Book} book
+     @return {Book}
+     */
+    addLanguageBook = function (language, book) {
+        let books = this.getBooks();
+        books = books.set(language, book);
+
+        return this.set("books", books);
+    };
+
+    /**
+     Set the summary for this book
+
+     @param {Summary}
+     @return {Book}
+     */
+    setSummary = function (summary) {
+        return this.set("summary", summary);
+    };
+
+    /**
+     Set the readme for this book
+
+     @param {Readme}
+     @return {Book}
+     */
+    setReadme = function (readme) {
+        return this.set("readme", readme);
+    };
+
+    /**
+     Set the configuration for this book
+
+     @param {Config}
+     @return {Book}
+     */
+    setConfig = function (config) {
+        return this.set("config", config);
+    };
+
+    /**
+     Set the ignore instance for this book
+
+     @param {Ignore}
+     @return {Book}
+     */
+    setIgnore = function (ignore) {
+        return this.set("ignore", ignore);
+    };
+
+    /**
+     Change log level
+
+     @param {String} level
+     @return {Book}
+     */
+    setLogLevel = function (level) {
+        this.getLogger().setLevel(level);
+        return this;
+    };
+
+    /**
+     Create a book using a filesystem
+
+     @param {FS} fs
+     @return {Book}
+     */
+
+    // @ts-expect-error ts-migrate(2339) FIXME: Property 'createForFS' does not exist on type 'Cla... Remove this comment to see the full error message
+    static createForFS = function createForFS(fs) {
+        return new Book({
+            fs: fs,
+        });
+    };
+
+    /**
+     Infers the default extension for files
+     @return {String}
+     */
+    getDefaultExt = function () {
+        // Inferring sources
+        const clues = [this.getReadme(), this.getSummary(), this.getGlossary()];
+
+        // List their extensions
+        const exts = clues.map((clue) => {
+            const file = clue.getFile();
+            if (file.exists()) {
+                return file.getParser().getExtensions().first();
+            } else {
+                return null;
+            }
+        });
+        // Adds the general default extension
+        exts.push(".md");
+
+        // Choose the first non null
+        return exts.find((e) => {
+            return e !== null;
+        });
+    };
+
+    /**
+     Infer the default path for a Readme.
+     @param {Boolean} [absolute=false] False for a path relative to
+     this book's content root
+     @return {String}
+     */
+    getDefaultReadmePath = function (absolute) {
+        const defaultPath = `README${this.getDefaultExt()}`;
+        if (absolute) {
+            return path.join(this.getContentRoot(), defaultPath);
+        } else {
+            return defaultPath;
+        }
+    };
+
+    /**
+     Infer the default path for a Summary.
+     @param {Boolean} [absolute=false] False for a path relative to
+     this book's content root
+     @return {String}
+     */
+    getDefaultSummaryPath = function (absolute) {
+        const defaultPath = `SUMMARY${this.getDefaultExt()}`;
+        if (absolute) {
+            return path.join(this.getContentRoot(), defaultPath);
+        } else {
+            return defaultPath;
+        }
+    };
+
+    /**
+     Infer the default path for a Glossary.
+     @param {Boolean} [absolute=false] False for a path relative to
+     this book's content root
+     @return {String}
+     */
+    getDefaultGlossaryPath = function (absolute) {
+        const defaultPath = `GLOSSARY${this.getDefaultExt()}`;
+        if (absolute) {
+            return path.join(this.getContentRoot(), defaultPath);
         } else {
-            return null;
+            return defaultPath;
         }
-    });
-    // Adds the general default extension
-    exts.push(".md");
-
-    // Choose the first non null
-    return exts.find((e) => {
-        return e !== null;
-    });
-};
-
-/**
- Infer the default path for a Readme.
- @param {Boolean} [absolute=false] False for a path relative to
- this book's content root
- @return {String}
- */
-Book.prototype.getDefaultReadmePath = function (absolute) {
-    const defaultPath = `README${this.getDefaultExt()}`;
-    if (absolute) {
-        return path.join(this.getContentRoot(), defaultPath);
-    } else {
-        return defaultPath;
-    }
-};
-
-/**
- Infer the default path for a Summary.
- @param {Boolean} [absolute=false] False for a path relative to
- this book's content root
- @return {String}
- */
-Book.prototype.getDefaultSummaryPath = function (absolute) {
-    const defaultPath = `SUMMARY${this.getDefaultExt()}`;
-    if (absolute) {
-        return path.join(this.getContentRoot(), defaultPath);
-    } else {
-        return defaultPath;
-    }
-};
-
-/**
- Infer the default path for a Glossary.
- @param {Boolean} [absolute=false] False for a path relative to
- this book's content root
- @return {String}
- */
-Book.prototype.getDefaultGlossaryPath = function (absolute) {
-    const defaultPath = `GLOSSARY${this.getDefaultExt()}`;
-    if (absolute) {
-        return path.join(this.getContentRoot(), defaultPath);
-    } else {
-        return defaultPath;
-    }
-};
-
-/**
- Create a language book from a parent
-
- @param {Book} parent
- @param {String} language
- @return {Book}
- */
-
-// @ts-expect-error ts-migrate(2339) FIXME: Property 'createFromParent' does not exist on type... Remove this comment to see the full error message
-Book.createFromParent = function createFromParent(parent, language) {
-    const ignore = parent.getIgnore();
-    let config = parent.getConfig();
-
-    // Set language in configuration
-    config = config.setValue("language", language);
-
-    return new Book({
-        // Inherits config. logegr and list of ignored files
-        logger: parent.getLogger(),
-        config: config,
-        ignore: ignore,
-
-        language: language,
-
-        // @ts-expect-error ts-migrate(2339) FIXME: Property 'reduceScope' does not exist on type 'Cla... Remove this comment to see the full error message
-        fs: FS.reduceScope(parent.getContentFS(), language),
-    });
-};
+    };
+
+    /**
+     Create a language book from a parent
+
+     @param {Book} parent
+     @param {String} language
+     @return {Book}
+     */
+
+    // @ts-expect-error ts-migrate(2339) FIXME: Property 'createFromParent' does not exist on type... Remove this comment to see the full error message
+    static createFromParent = function createFromParent(parent, language) {
+        const ignore = parent.getIgnore();
+        let config = parent.getConfig();
+
+        // Set language in configuration
+        config = config.setValue("language", language);
+
+        return new Book({
+            // Inherits config. logegr and list of ignored files
+            logger: parent.getLogger(),
+            config: config,
+            ignore: ignore,
+
+            language: language,
+
+            // @ts-expect-error ts-migrate(2339) FIXME: Property 'reduceScope' does not exist on type 'Cla... Remove this comment to see the full error message
+            fs: FS.reduceScope(parent.getContentFS(), language),
+        });
+    };
+}

 export default Book;

[azu]

Migrate models to class-based for improving TypeScript compatibility This issue is separated from this issue,

→ Move to #169