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 double chaining stack overflow #2963

Closed
wants to merge 1 commit into from
Closed

Fix double chaining stack overflow #2963

wants to merge 1 commit into from

Conversation

j0pgrm
Copy link

@j0pgrm j0pgrm commented Aug 31, 2022

@jgonggrijp
Copy link
Collaborator

Thank you @j0pgrm for making a contribution! The approach you took is not quite right, as it breaks functionality. I'll help you find a better solution.

I suggest you take the following steps:

  • Inside the root directory of your local clone of the Underscore project, on your own double-chaining branch, run npm install. This will install all the tools we use during Underscore development.
  • Run npm run test. You will see that six tests fail, due to your changes.
  • Run git revert HEAD. This will create a new commit that restores the situation before your change.
  • Run npm run test again. Now, all the tests pass (although we know there is a bug, which is Double chaining leads to a stack overflow #2853 and which you are trying to fix).
  • Add a new test which exposes the bug. It should do something like var testValue = _.chain(1).chain() and then assert that testValue.value() is equal to 1. Since the bug is not solved yet, we expect this test to fail initially. Commit this change.
  • Run npm run test again and verify that your new test fails, while all other tests pass.
  • Open test/index.html in your browser. This will run the tests in the browser and should give you the same result.
  • Use the developer tools of your browser to set a breakpoint at the start of your new test. If you have not done this before, here is a tutorial for Firefox and here is one for Chrome. Step through the code to see what is causing the bug. Do this repeatedly until you fully understand what is going on.
  • Post a comment here to explain what you observed and what you think is causing the bug.

After the above steps, we'll discuss your findings and possible solutions.

@jgonggrijp jgonggrijp changed the title deploy Fix double chaining stack overflow Aug 31, 2022
@jgonggrijp jgonggrijp linked an issue Aug 31, 2022 that may be closed by this pull request
@jgonggrijp jgonggrijp marked this pull request as draft August 31, 2022 11:45
@j0pgrm j0pgrm closed this by deleting the head repository Sep 27, 2022
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.

Double chaining leads to a stack overflow
2 participants