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

feat: add support for private class methods #4034

Merged
merged 3 commits into from Apr 9, 2021

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Apr 5, 2021

add support for private methods: https://github.com/tc39/proposal-private-methods

REPL

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

@github-actions
Copy link

github-actions bot commented Apr 5, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install dnalborczyk/rollup#private-methods

Once a build has completed, you can load it into the REPL by inserting the CircleCI build number of the ci/circleci: node-v12-latest build into the link below (unfortunately, we cannot auto-update this comment for PRs from forks at this time):

https://rollupjs.org/repl/?circleci=<build_number>

@dnalborczyk dnalborczyk marked this pull request as draft April 5, 2021 18:37
@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #4034 (5ae5dfb) into master (dba6f13) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4034   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files         192      192           
  Lines        6785     6785           
  Branches     1994     1994           
=======================================
  Hits         6614     6614           
  Misses         84       84           
  Partials       87       87           
Impacted Files Coverage Δ
src/utils/options/normalizeInputOptions.ts 100.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 dba6f13...5ae5dfb. Read the comment docs.

@dnalborczyk dnalborczyk marked this pull request as ready for review April 5, 2021 21:58
@dnalborczyk
Copy link
Contributor Author

ci on windows seems to have an issue:

npm ERR! Error while executing:
npm ERR! C:\Program Files\Git\bin\git.EXE ls-remote -h -t ssh://git@github.com/lukastaegert/is-reference.git
npm ERR! 
npm ERR! Host key verification failed.
npm ERR! fatal: Could not read from remote repository.
npm ERR! 
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR! 
npm ERR! exited with error code: 128

@lukastaegert
Copy link
Member

I assume you are not using SSH to access Github? That could explain the test issues. Will see if we can change something on our side to improve here.

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Apr 8, 2021

I assume you are not using SSH to access Github? That could explain the test issues. Will see if we can change something on our side to improve here.

sorry, I was referring to the windows ci run: https://github.com/rollup/rollup/runs/2273299082 not any local runs (I'm using a mac).

my local git settings wouldn't have any effect on how the ci pulls the dependency repo? 🤔

@lukastaegert
Copy link
Member

Probably not. Still I wonder why it could not fetch the dependency via SSH, my though is that it was using some settings from your fork, but then again, it is very weird. Others doing that did not have this issue. In any case, I fixed it by changing the dependency to force HTTPS instead of SSH, this should always work.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Very nice, and glad you could finally remove the comments from the test cases!

@lukastaegert lukastaegert merged commit 35840ed into rollup:master Apr 9, 2021
@dnalborczyk dnalborczyk deleted the private-methods branch April 9, 2021 12:39
@jef
Copy link

jef commented Apr 9, 2021

Did it have anything to do with the way npm 7 vs npm 6 worked in pulling the dependencies? I see the package-lock changed between the two as well.

@lukastaegert
Copy link
Member

Possible. For now I did not dare updating the lock file. But more importantly, the original lock file contained an ssh references while the new one uses https for the is_references package.

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Apr 9, 2021

But more importantly, the original lock file contained an ssh references while the new one uses https for the is_references package.

ah, I see, that was the issue then! yeah, I was using node.js v15+ w/ npm 7+. forgot the fact that they updated the package-lock format (v2).

thank you for looking into this @jef !

thank you for reviewing and pulling in the PR @lukastaegert !

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.

None yet

3 participants