grantcarthew / node-rethinkdb-job-queue

A persistent job or task queue backed by RethinkDB.
https://github.com/grantcarthew/node-rethinkdb-job-queue/wiki
MIT License
156 stars 16 forks source link

includes is not a function #28

Closed thomasmodeneis closed 7 years ago

thomasmodeneis commented 7 years ago

Hi,

When I run this module, I'm forced to use node --harmony_array_includes to have it running, otherwise I'm getting:

 if (Object.keys(enums.priority).includes(oldOptions.priority)) {
                                  ^
TypeError: Object.keys(...).includes is not a function
    at jobOptions (/opt/.../node_modules/rethinkdb-job-queue/dist/job-options.js:19:35)
    at new Queue (/opt/.../node_modules/rethinkdb-job-queue/dist/queue.js:55:25)

Any ideas ?

Thanks

thomasmodeneis commented 7 years ago

Oh right this merge explains why. https://github.com/nodejs/node/issues/5715#issuecomment-196745766

I believe it will be nice to warn people about this, just in case they stump upon it.

grantcarthew commented 7 years ago

Hi @thomasmodeneis. Thanks for the report. I am compiling the queue using Babel and it should support all versions of node. I will look into adding a Babel plugin once I have finished working on #26.

grantcarthew commented 7 years ago

@thomasmodeneis, I have switched to using babel-preset-latest which should fix your issue.

Upgrade to v1.0.1 and see if the issue is gone please.

thomasmodeneis commented 7 years ago

Nice, I will check asap. Cheers. :)

thomasmodeneis commented 7 years ago

hi @grantcarthew I've tested but the issue remains. I'm on

$ node --version
v5.6.0
grantcarthew commented 7 years ago

OK @thomasmodeneis, I posted a message on the Babel slack channel and learned that Babel only transforms syntax and will not pollyfill.

Can I ask for you help here please? Can you install the babel-pollyfill locally (no need to save) and add the require("babel-polyfill") statement to the top of the dist/queue.js file in your local package of rethinkdb-job-queue? You will need to find the local package in your node_modules directory.

Here are the details about the pollyfill: http://babeljs.io/docs/usage/polyfill/

I can give more detailed instructions if that is not enough to help.

hzoo commented 7 years ago

http://babeljs.io/docs/usage/polyfill/ or you can install the specific polyfill for https://www.npmjs.com/package/array-includes

grantcarthew commented 7 years ago

I think there will be more than just the includes issue @hzoo. That's up to you @thomasmodeneis if you want to try and fix each pollyfill.

Being that this is server side code, the extra package size is probably a non-issue.

I am starting to use more and more of the newer ES standards so the full pollyfill will prevent regressions.

The other option is to forget about transpiling and determine the minimum Node.js version required and just make is an install requirement.

thomasmodeneis commented 7 years ago

I will have a look on the pollyfill thing asap. About the minimum required version, are you planning to block people to install the module if they are lets say using a older version ? Or the idea is to add a warn and alert users that they should be using the flag --harmony_array_includes ? Cheers.

grantcarthew commented 7 years ago

Either or, I did not really put much thought into not transpiling. Seems like a backward step.

grantcarthew commented 7 years ago

@thomasmodeneis Hey mate, any update on this?

thomasmodeneis commented 7 years ago

@grantcarthew Sorry for the delay, I got this module in prod for one of my systems and well there was other issues hanging my eyes for the week :D

I've tested adding require("babel-polyfill") and it works like you said, so I guess this is a fix! 👍

grantcarthew commented 7 years ago

Great. Nice work @thomasmodeneis.

This is a quote from the pollyfills site:

Note: Depending on what ES2015 methods you actually use, you may not need to use babel-polyfill or the runtime plugin. You may want to only load the specific polyfills you are using (like Object.assign) or just document that the environment the library is being loaded in should include certain polyfills.

I think the last sentence is the kicker. I will put a note on the readme about Node environment issues and point to the pollyfill.

Thanks again for reporting this.

grantcarthew commented 7 years ago

I've added a note to the Readme under installation. As I was writing it, it occurred to me that you could just add the Pollyfill at the root of your code, not the rethinkdb-job-queue module code.

Have a good day/night.

thomasmodeneis commented 7 years ago

I'm not sure if the readme is saying the right thing. You said: "WARNING: If you are using an older version of Node.js you may need the Babel Pollyfill. "

You said on the readme file: You may need "Babel Pollyfill". Could you be more specific ? what is "older version" ? And what is the recommended version ? Also you don't mention that user will have to use this in every class that will be making use of this lib and that the user should add the require("babel-polyfill") statement to the top of every file using node-rethinkdb-job-queue.

As well this is not the only way to work around the issue, also running it with "node --harmony_array_includes" is a valid work around.

Cheers.

grantcarthew commented 7 years ago

I see your point @thomasmodeneis. I'll expand on it.

grantcarthew commented 7 years ago

Added an installation document to the Wiki. Reference it on the README.

thomasmodeneis commented 7 years ago

@grantcarthew you are very proactive mate, I like it! Thanks and have a great week.