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 option to disallow code generation from strings #144

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link

@anonrig anonrig commented Nov 4, 2024

eval and new Function is not available on platforms such as Cloudflare workers. This option will add support for disabling and testing environments where code generation from strings are not available.

@anonrig anonrig force-pushed the enable-disallow-code-generation-from-strings branch from 9f6ec36 to c69effe Compare November 4, 2024 13:11
@kibertoad
Copy link
Member

Thanks! can you add documentation for this?

@anonrig anonrig force-pushed the enable-disallow-code-generation-from-strings branch from c69effe to a8f47a6 Compare November 4, 2024 13:14
@anonrig
Copy link
Author

anonrig commented Nov 4, 2024

Thanks! can you add documentation for this?

I updated the README with the newly added option. Let me know if there's anything missing, and thanks for the review @kibertoad

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 4, 2024

well this would break fast-json-stringify and all dependents on it.

@anonrig
Copy link
Author

anonrig commented Nov 4, 2024

well this would break fast-json-stringify and all dependents on it.

I'll open a different pull-request and enable it there. The default value is false and it won't break anything unless we enable it.

Copy link

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Wow. Thanks for a speedy turnaround @anonrig!

I left a comment/wuestion but overall LGTM!

@@ -110,12 +115,23 @@ jobs:
matrix:
node-version: ${{ fromJson(inputs.node-versions) }}
os: [macos-latest, ubuntu-latest, windows-latest]
disallow-code-generation-from-strings: ${{ inputs.check-disallow-code-generation-from-strings == true && ['true', 'false'] || ['false'] }}

Choose a reason for hiding this comment

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

Is this the simplest way to express this? Apologies for ignorance but I find this expression hard to follow without a comment, I wonder if other readers if this code feel the same.

@@ -110,12 +115,23 @@ jobs:
matrix:
node-version: ${{ fromJson(inputs.node-versions) }}
os: [macos-latest, ubuntu-latest, windows-latest]
disallow-code-generation-from-strings: ${{ inputs.check-disallow-code-generation-from-strings == true && ['true', 'false'] || ['false'] }}

Choose a reason for hiding this comment

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

Is this the simplest way to express this? Apologies for ignorance but I find this expression hard to follow without a comment, I wonder if other readers if this code feel the same.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 4, 2024

Dont get me wrong, but I would expect this as a npm script.

@@ -130,7 +146,7 @@ jobs:
run: npm i --ignore-scripts

- name: Run tests
run: npm test
run: NODE_OPTIONS="${{ steps.node-flags.outputs.flags }}" npm test
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this as a different job, not as an edit of this one.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't understand. Since it is in a matrix config, it will run 2 different jobs. 1 for false value, and one for true. (or only 1 if it's not enabled)

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.

5 participants