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. New option ignoreFunction according to issue#32 #58

Merged
merged 7 commits into from Sep 4, 2019

Conversation

realdennis
Copy link
Contributor

@realdennis realdennis commented Sep 2, 2019

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

According to this issue #32 , I add a new option ignoreFunction for serialize.

function fn(){ return true; }
serialize( fn , { ignoreFunction:true}) // 'undefined'
serialize({
    num:123,
    fn
},{ignoreFunction:true}); // `{"num":123}`
  • Add feature
  • Write test case
  • Update document

index.js Outdated
@@ -31,6 +31,18 @@ function escapeUnsafeChars(unsafeChar) {
return ESCAPED_CHARS[unsafeChar];
}

function deleteFunctions(obj){
var functionKeys = []
for(var key in obj){
Copy link
Contributor Author

@realdennis realdennis Sep 2, 2019

Choose a reason for hiding this comment

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

Actually I wrote this first

    Object.keys(obj)
    .filter(key => typeof obj[key] === "function")
    .forEach(functionKey => {
      delete obj[functionKey];
    });

But I have no idea what the lowest browser the code should support to,
so I avoid to use Object.keys

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@tom76kimo
Copy link

@tom76kimo tom76kimo commented Sep 2, 2019

Could @redonkulus help review this?

@realdennis
Copy link
Contributor Author

@realdennis realdennis commented Sep 2, 2019

@tom76kimo Thanks for reminding me.

README.md Show resolved Hide resolved
test/unit/serialize.js Outdated Show resolved Hide resolved
test/unit/serialize.js Outdated Show resolved Hide resolved
test/unit/serialize.js Outdated Show resolved Hide resolved
test/unit/serialize.js Outdated Show resolved Hide resolved
test/unit/serialize.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@realdennis
Copy link
Contributor Author

@realdennis realdennis commented Sep 2, 2019

@okuryu Updated, thanks!

@realdennis
Copy link
Contributor Author

@realdennis realdennis commented Sep 2, 2019

@okuryu By the way, there is some inconsistent style in the original code.

2 space indents
https://github.com/yahoo/serialize-javascript/blob/master/index.js#L91-L95

4 space indents
https://github.com/yahoo/serialize-javascript/blob/master/index.js#L51-L60

Maybe we should make a new pull request or issue to discuss about coding style, like lint check using ESLint, and some helpful config like .eslintrc and .prettierrc in the future.

@okuryu
Copy link
Collaborator

@okuryu okuryu commented Sep 2, 2019

yeah, I'll try to format with prettier separately.

okuryu
okuryu approved these changes Sep 2, 2019
@okuryu
Copy link
Collaborator

@okuryu okuryu commented Sep 2, 2019

Okay, I'll work on merging and release on this until the end of tomorrow If there are no other comments.

@okuryu okuryu merged commit 9b47a96 into yahoo:master Sep 4, 2019
1 check passed
@okuryu
Copy link
Collaborator

@okuryu okuryu commented Sep 4, 2019

published v2.1.0. Thanks!

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