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 onSanitize and dryRun option #39

Merged
merged 2 commits into from
Mar 18, 2021
Merged

Conversation

azu
Copy link
Collaborator

@azu azu commented Jan 18, 2021

  • Add onSanitize option
  • Add dryRun option

The onSanitize callback will be called even if the dryRun is true.
It aims to remove a built-in warning like #35 (comment) from this module

fix #35

}

function sanitize(target, options) {
return _sanitize(target, options).target;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This _sanitize aims to preserve backward compatibility…

* @param {{replaceWith?: string, onSanitize?: function, dryRun?: boolean}} options
* @returns {function}
*/
function middleware(options = {}) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just use default parameter instead of options = options || {};

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

@@ -5,6 +5,17 @@ const express = require('express');
const bodyParser = require('body-parser');
const expect = require('chai').expect;
const sanitize = require('./index.js');
const callTracker = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Node.js 12+ has https://nodejs.org/api/assert.html#assert_class_assert_calltracker
But Node.js 10 does not this.

@azu azu mentioned this pull request Jan 18, 2021
@azu
Copy link
Collaborator Author

azu commented Jan 25, 2021

@fiznool Can you review it?

@fiznool
Copy link
Owner

fiznool commented Jan 25, 2021

@azu sorry for the delay, I've been off sick the past few days. I will try to take a look this week, thank you for your patience!

@azu
Copy link
Collaborator Author

azu commented Mar 18, 2021

If you are busy, I can help you to maintain this library.

@fiznool fiznool merged commit 322c0f1 into fiznool:master Mar 18, 2021
@fiznool
Copy link
Owner

fiznool commented Mar 18, 2021

Sorry for the delay. I've merged this in and will release a new minor version once I've updated the dependencies.

Help with this module would be appreciated - I will add you to the repo?

@azu azu deleted the onSanitize branch March 18, 2021 06:50
@azu
Copy link
Collaborator Author

azu commented Mar 18, 2021

Thanks for inviting me!

I will help to merge #41 and support TypeScript(rewritten with TS or just add .d.ts).

@fiznool
Copy link
Owner

fiznool commented Mar 18, 2021

TypeScript support would be awesome. I'd prefer to go for the separate ambient types, rewrites often make me nervous!

@azu azu mentioned this pull request Mar 18, 2021
@azu
Copy link
Collaborator Author

azu commented Mar 18, 2021

I'd prefer to go for the separate ambient types, rewrites often make me nervous!

OK! I've created an issue #55

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.

reporting callback
2 participants