Skip to content

remove coffeescript dependency and use vanilla .js files #349

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

Merged
merged 19 commits into from
Feb 29, 2020

Conversation

kaizhu256
Copy link
Member

this pull-request is broken up into 4 commits:

1) create vanilla .js files from .coffee by running shell-command:

# revert individual .coffee files to .js
npm install [email protected]
for LIB in api-data api exports worker
do
    cat "src/$LIB.coffee" | npx coffee --bare --compile --stdio > "src/$LIB.js"
done
# restore package-lock.json
git checkout package-lock.json

2) update Makefile to use vanilla src/.js files instead of src/.coffee

3) remove references and dependencies to coffeescript in files:

$ git grep -i coffee
.github/workflows/CI.yml:    - name: install coffee
.github/workflows/CI.yml:      run: npm install --global coffeescript@1
.github/workflows/release.yml:      - name: install coffee
.github/workflows/release.yml:        run: npm install --global coffeescript@1
.npmignore:coffee/
package-lock.json:              "coffeescript": {
package-lock.json:                      "resolved": "https://registry.npmjs.org/coffeescript/-/coffeescript-1.12.7.tgz",
package.json:           "coffeescript": "^1.12.7",

4) remove .coffee files:

deleted:    src/api-data.coffee
deleted:    src/api.coffee
deleted:    src/exports.coffee
deleted:    src/worker.coffee

kai.zhu added 4 commits February 24, 2020 17:45
```sh
# revert individual .coffee files to .js
npm install [email protected]
for LIB in api-data api exports worker
do
    cat "src/$LIB.coffee" | npx coffee --bare --compile --stdio > "src/$LIB.js"
done
# restore package-lock.json
git checkout package-lock.json
```
```
$ git grep -i coffee
.github/workflows/CI.yml:    - name: install coffee
.github/workflows/CI.yml:      run: npm install --global coffeescript@1
.github/workflows/release.yml:      - name: install coffee
.github/workflows/release.yml:        run: npm install --global coffeescript@1
.npmignore:coffee/
package-lock.json:              "coffeescript": {
package-lock.json:                      "resolved": "https://registry.npmjs.org/coffeescript/-/coffeescript-1.12.7.tgz",
package.json:           "coffeescript": "^1.12.7",
```
        deleted:    src/api-data.coffee
        deleted:    src/api.coffee
        deleted:    src/exports.coffee
        deleted:    src/worker.coffee
@lovasoa
Copy link
Member

lovasoa commented Feb 24, 2020

Hello and thank you for this PR !
I am not against moving away from coffeescript. The decision to use it was taken several years ago, when javascript was not what it is today.
But we can't keep the raw converted files, which are not human-readable, maintainable javascript. We'll have to clean up the :

var _, binaryDb, func, ref, ref1, stmt;
    ref = this.statements;
    for (_ in ref) {
      stmt = ref[_];
      stmt['free']();
    }

or

        data_func = (function() {
          switch (false) {
            case value_type !== 1:
              ...
            case value_type !== 4:
              return function(ptr) {
                var blob_arg, blob_ptr, j, l, ref1, size;
                ...
                for (j = l = 0, ref1 = size; 0 <= ref1 ? l < ref1 : l > ref1; j = 0 <= ref1 ? ++l : --l) ...
                return blob_arg;
              };
            ...
          }
        })();

And since manual work will be required on all of the source files, I thought we may as well add type information and switch to Typescript. Would you be interested in such a project @kaizhu256 ?

@lovasoa
Copy link
Member

lovasoa commented Feb 24, 2020

The port to typescript was already implemented in #290 by @pwnsdx

@kaizhu256
Copy link
Member Author

And since manual work will be required on all of the source files, I thought we may as well add type information and switch to Typescript. Would you be interested in such a project @kaizhu256 ?

i'm not really a typescript fan ^^;;; the intent of pull-request is to make project more accessible to people put off by excessive tooling (including typescript).

i'll be happy to refactor the .coffee files to [mostly] pass jslint. yea, i kno jslint is tooling as well, but at least its human-readable vanilla-js.

@lovasoa
Copy link
Member

lovasoa commented Feb 25, 2020

OK, let's first clean up the javascript, we'll then see about whether we want to add type information.

@kaizhu256 kaizhu256 force-pushed the remove.dependency.coffeescript branch 7 times, most recently from d8964ee to d3836ee Compare February 27, 2020 08:31
…nto api.js, respectively:

    src/output-pre.js
    src/api.js
    src/exports.js
    src/api-data.js
    src/output-post.js
@kaizhu256 kaizhu256 force-pushed the remove.dependency.coffeescript branch from 3feb990 to b8c30ed Compare February 27, 2020 09:46
@kaizhu256
Copy link
Member Author

add 2 more stable commits (6 total):

commit (5)

simplify ci-build and vanilla-js cleanup by merging following files into api.js, respectively:

        src/output-pre.js
        src/api.js
        src/exports.js
        src/api-data.js
        src/output-post.js

commit (6)

rename OUTPUT_API_FILES to SOURCE_API_FILES in Makefile

@lovasoa
Copy link
Member

lovasoa commented Feb 27, 2020

If our code passes jslint, I think we should add jslint to the devdependencies and to the tests

Copy link
Member

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think we are starting to see clearer. There are still a few coffeescriptisms to remove and we'll be done.
Especially, a lot of loops can be simplified, ref variables can be inlined, and var keywords can be put directly where the variables are declared.

todo - allow test-jslint to fail after we finish refactoring src/api.js (test-jslint currently always pass)
lint src/api.js - resolve conversation - Object.values()
lint src/api.js - resolve conversation - we can iterate in increasing order
lint src/api.js - resolve conversation - the var could be directly in front of the variables
lint src/api.js - resolve conversation - remove merge-comments
lint src/api.js - unindent case-statement
lint src/api.js - use jslint-style-guide of quoting strings with double-quote instead of single-quote
lint src/api.js - hoist constant vars to top of file
lint src/api.js - rename property SQlite.xxx to var SQLITE_xxx
lint src/api.js - remove var runCompiledCode
lint src/api.js - use 4-space indent
@kaizhu256 kaizhu256 force-pushed the remove.dependency.coffeescript branch from a316370 to 809a47f Compare February 28, 2020 17:37
@kaizhu256
Copy link
Member Author

add stable commit # 7 (eccffee)

add file test/jslint.js and npm-command "npm run test-jslint"
todo - allow test-jslint to fail after we finish refactoring src/api.js (test-jslint currently always pass)

add stable commit # 8 (809a47f)

lint src/api.js - resolve conversation - we need only 1 variable
lint src/api.js - resolve conversation - Object.values()
lint src/api.js - resolve conversation - we can iterate in increasing order
lint src/api.js - resolve conversation - the var could be directly in front of the variables
lint src/api.js - resolve conversation - remove merge-comments
lint src/api.js - unindent case-statement
lint src/api.js - use jslint-style-guide of quoting strings with double-quote instead of single-quote
lint src/api.js - hoist constant vars to top of file
lint src/api.js - rename property SQlite.xxx to var SQLITE_xxx
lint src/api.js - remove var runCompiledCode
lint src/api.js - use 4-space indent

…defined(xxx)"

jslint src/api.js - replace expression "void 0" with "undefined"
jslint src/api.js - fix warning "Line is longer than 80 characters."
jslint src/api.js - fix warning "Undeclared xxx."
jslint src/api.js - fix miscellaneous jslint warnings in src/api.js
jslint - allow jslint to print all warnings
jslint - ignore warning subscript_a
@kaizhu256 kaizhu256 force-pushed the remove.dependency.coffeescript branch from 81174ca to 37b0c58 Compare February 28, 2020 20:10
jslint - "npm test" will now fail if src/api.js fails jslint-check
jslint src/api.js - fix whitespace warnings
@kaizhu256
Copy link
Member Author

add stable commit # 9 (9afe9f1)

jslint src/api.js - fixed all warnings
jslint - "npm test" will now fail if src/api.js fails jslint-check
jslint src/api.js - fix whitespace warnings

@lovasoa, i think this pull-request is ready for merge on my side. can you review when you have time?
also, can we skip refactoring and regression-testing for src/worker.js in a separate pull-request ?

@lovasoa
Copy link
Member

lovasoa commented Feb 28, 2020

Thank you for your fantastic work, @kaizhu256 !
I am going to try and find the time to work on this during this weekend.
I don't think this is ready for merging right now. In particular, we cannot afford to check in and maintain a fork of jslint.
I have access to your branch so I can work on this PR directly, and keep this in the form of a PR until this is ready for merging.

@kaizhu256
Copy link
Member Author

ok and appreciate the thx : ) i'm excited and sincerely believe sql.js is the future of state-management in browsers!

@lovasoa lovasoa merged commit ca115e1 into sql-js:master Feb 29, 2020
lovasoa added a commit that referenced this pull request Feb 29, 2020
Was left there after #349
@kaizhu256 kaizhu256 mentioned this pull request Mar 1, 2020
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.

2 participants