Skip to content

Conversation

@sakshisharmapuresoftware
Copy link
Contributor

Phantomjs is an unmaintained project, replaced the phantomjs dependency with the puppeteer.
Updated the travis.yml to remove the nodejs version 4 as mentioned in #398.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.122% when pulling 7502bf0 on sakshi87:formdata into 5c6bf52 on form-data:master.

@mapleeit
Copy link
Collaborator

mapleeit commented Sep 3, 2019

LGTM. But removing support for Node 4 should be a breaking change.

@mapleeit
Copy link
Collaborator

mapleeit commented Sep 3, 2019

Does puppeteer must run at least NodeJS v6?
If not, we should consider separating these two changes.

@sakshisharmapuresoftware
Copy link
Contributor Author

@mapleeit As you can check, the Travis-ci builds the package is working fine on NodeJS v6 as well with the puppeteer.

I will be happy to update the PR though if you want these two changes separated, Please consider merging this PR.

@mapleeit
Copy link
Collaborator

mapleeit commented Sep 3, 2019

I mean can the code still runs on Node4 after changing? If it's not, then it is a breaking change. We should be carefully of that.

@sakshisharmapuresoftware
Copy link
Contributor Author

No, it doesn't work after change on Node v4, I agree this is a breaking change but the older node version is not supported by the puppeteer.
Below is the error which I am getting on node v4:

/Users/travis/build/sakshi87/form-data/node_modules/puppeteer/install.js:78
  return Promise.all([...cleanupOldVersions, generateProtocolTypesIfNecessary(true /* updated */)]);
                      ^^^
SyntaxError: Unexpected token ...

npm WARN engine [email protected]: wanted: {"node":">=6.4.0"} (current: {"node":"4.9.1","npm":"2.15.11"})

The puppeteer atleast requires node v6, that's why it is giving issue on node v4.

Please suggest what needs to be done to get this PR merged.

@mapleeit
Copy link
Collaborator

mapleeit commented Sep 4, 2019

@alexindigo Please take a look when you have time. I'm not sure about how to handle this.

@alexindigo
Copy link
Member

@mapleeit @sakshi87 Thanks for looking into and pushing it through. Node4 is deprecated, so it's ok to drop it, but as you pointed out it's a breaking change. So how about bumping all the dependencies and releasing version 3?

Thank you.

@mapleeit
Copy link
Collaborator

mapleeit commented Sep 4, 2019

Sounds good to me. In addition, we should also update engines.node in package.json.

@mapleeit mapleeit added the v3.0 label Sep 19, 2019
@mapleeit mapleeit merged commit a73a53c into form-data:master Oct 30, 2019
@mapleeit mapleeit mentioned this pull request Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants