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

refactor: get rid of coffeescript #29

Merged
merged 3 commits into from
Jul 13, 2017
Merged

refactor: get rid of coffeescript #29

merged 3 commits into from
Jul 13, 2017

Conversation

dbushong
Copy link
Member

  • port src/assertive.coffee to readable lib/assertive.js
  • port test/*.test.coffee to js
  • remove coffee dependencies & mocha linkage
  • remove build step
  • update README.md
  • install working eslint modules & rules

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

Some small questions but looks great overall!

package.json Outdated
"test": "mocha",
"posttest": "nlm verify",
"test-run": "coffeelint src test && mocha",
"watch": "coffee --no-header -wcbo lib src & nodemon -w lib -w test -e coffee,js,json -x \"mocha\""
"test-run": "eslint lib test && mocha"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still a thing? Looks like this effectively does the same as npm test?

// test the auto-promise-awarified versions of common tests

const Bluebird = require('bluebird');
const assert = require('../lib/assertive');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we generally prefer require('../') to ensure that the entry point works correctly.


// helper from decaffeinate - useful for now
/* eslint-disable */
function __range__(left, right, inclusive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add lodash to the test dependencies for this?

* port src/assertive.coffee to readable lib/assertive.js
* port test/*.test.coffee to js
* remove coffee dependencies & mocha linkage
* remove build step
* update README.md
* install working eslint modules & rules
@dbushong dbushong merged commit a704390 into master Jul 13, 2017
@dbushong dbushong deleted the remove-coffee branch July 13, 2017 20:46
@aaarichter
Copy link

aaarichter commented Jul 14, 2017

there is an error being thrown when running test-client with node 6.x

not ok 1 PhantomJS 2.1 - Global error: SyntaxError: Unexpected token 'const' at http://localhost:7357/test/tmp/test-bundle.js, line 1310
    ---
        Log: |
            { type: 'error',
              text: 'SyntaxError: Unexpected token \'const\' at http://localhost:7357/test/tmp/test-bundle.js, line 1310\n' }
    ...

test-bundle.js:

},{}],16:[function(require,module,exports){
(function (process){
'use strict';

/*
...
*/

// eat _ off the global scope, or require it ourselves if missing
// eslint-disable-next-line no-new-func
const global = Function('return this')();
let assert;

// eslint-disable-next-line global-require
const _ = global._ || require('lodash');
...

@dbushong
Copy link
Member Author

D'oh. OK, pin the old version for the moment - I guess we'll use babel ;;

@dbushong
Copy link
Member Author

@aaarichter should be fix as of #31 - will ship in a sec

@aaarichter
Copy link

Hey David - no problem. A hint to use babel should do it too 😉

We will add babel to our js asset pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants