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

unassigned var is undefined #125

Merged
merged 3 commits into from
Oct 3, 2019

Conversation

pop
Copy link
Contributor

@pop pop commented Oct 1, 2019

Fixes #113

Just like the original issue claimed, this was a two line fix...

... but I didn't stop there! I don't feel comfortable making a change without tests, so I built out the tests/js/test.js file to (A) include a test for my fix and (B) include some scaffolding for future JS smoke tests.

If you think this needs some changes, feel free to ping me or make some commits on my branch. I'd also be willing to remove the testing changes. They don't really "fit" in this pull request and I'd understand if you are philosophically opposed to them.

@jasonwilliams
Copy link
Member

jasonwilliams commented Oct 1, 2019

@pop this is awesome!
however that file isn't really for tests (i should probably add it to the gitignore).
Its just there for people to try out JS and run it.

If you want to add the tests you're best off adding them at the bottom of exec.rs
Here are some examples:
https://github.com/jasonwilliams/boa/blob/master/src/lib/js/boolean.rs#L148-L166
https://github.com/jasonwilliams/boa/blob/master/src/lib/js/string.rs#L809-L830

You can do something similar to those on exec.rs

@pop
Copy link
Contributor Author

pop commented Oct 2, 2019

@jasonwilliams Good to know! I added two tests to the bottom of exec.rs.

Let me know if you want me to remove the changes to tests/js/test.js, or if you want me to add something to the CHANGELOG for this. And of course any other changes you'd like.

@jasonwilliams
Copy link
Member

@pop if you could update the CHANGELOG and revert the test.js changes that would be great

@jasonwilliams
Copy link
Member

@pop sorry ive merged since so you may need to rebase

@pop pop force-pushed the bug/unassigned-var-undefined branch from af8e876 to 7aa105d Compare October 3, 2019 14:02
@pop pop force-pushed the bug/unassigned-var-undefined branch from 7aa105d to 35d2d47 Compare October 3, 2019 14:17
@pop pop force-pushed the bug/unassigned-var-undefined branch from 35d2d47 to 2c44735 Compare October 3, 2019 14:32
@pop
Copy link
Contributor Author

pop commented Oct 3, 2019

@jasonwilliams I think it's ready 😄

Added some CHANGELOG text, removed my tests/js/test.js code and added a comment to that file to help future me, and fixed my tests.

@jasonwilliams jasonwilliams merged commit d798566 into boa-dev:master Oct 3, 2019
DomParfitt pushed a commit to DomParfitt/boa that referenced this pull request Oct 6, 2019
* Unassigned variables are set to `undefined` not `null`

Fixes boa-dev#113

* Rust tests for `var x` and `let x` default to undefined

* CHANGELOG for issue boa-dev#113 fix + add tests/js/test.js to gitignore.
jasonwilliams pushed a commit that referenced this pull request Oct 11, 2019
* Added create_constructor method

* Updated global to use json::create_constructor method

* unassigned var is undefined (#125)

* Unassigned variables are set to `undefined` not `null`

Fixes #113

* Rust tests for `var x` and `let x` default to undefined

* CHANGELOG for issue #113 fix + add tests/js/test.js to gitignore.

* fixing PR benchmarks (#132)

* fixing PR benchmarks

* Push after rebase

* Updated import order

* Formatting
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.

Unassigned variables have default of null instead of undefined
2 participants