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

fix: npm installation error #1697

Merged
merged 3 commits into from
Jul 16, 2021
Merged

fix: npm installation error #1697

merged 3 commits into from
Jul 16, 2021

Conversation

rubiin
Copy link
Member

@rubiin rubiin commented Jul 16, 2021

Updated rollup to solve installation error

Checklist

  • PR contains only changes related; no stray files, etc.
  • [] README updated (where applicable)
  • [] Tests written (where applicable)

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #1697 (e34c376) into master (b82d4e1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1697   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          101       101           
  Lines         1855      1862    +7     
=========================================
+ Hits          1855      1862    +7     
Impacted Files Coverage Δ
src/lib/isBoolean.js 100.00% <0.00%> (ø)
src/lib/isLicensePlate.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b82d4e1...e34c376. Read the comment docs.

@rubiin
Copy link
Member Author

rubiin commented Jul 16, 2021

rollup latest version doesnot seem to support older versions. Also node 6.0 is way old i think we should drop tests for it as its likely someone is using that

Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Even if node 6 may not be used anymore this is a breaking change and should be released in a new major.

@rubiin
Copy link
Member Author

rubiin commented Jul 16, 2021

Even if node 6 may not be used anymore this is a breaking change and should be released in a new major.

however its a blocker if you cant install the package dependencies after forking. @profnandaa

@rubiin
Copy link
Member Author

rubiin commented Jul 16, 2021

tested on node v10,12 and v14.

@profnandaa
Copy link
Member

Don't leave out Node 6 yet, there's a time it helped us catch an old browser incompatibility, see #1357

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Jul 16, 2021
@rubiin
Copy link
Member Author

rubiin commented Jul 16, 2021

@profnandaa fixed it without breaking support for node 6

@profnandaa
Copy link
Member

@rubiin -- thanks!

@profnandaa profnandaa merged commit cff8a2e into validatorjs:master Jul 16, 2021
@rubiin
Copy link
Member Author

rubiin commented Jul 16, 2021

@profnandaa should i also make a pr to update eslint and some tools. They are way old

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants