-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Added jsescOptions to Numeric Literals #11028
Added jsescOptions to Numeric Literals #11028
Conversation
edited by nicolo-ribaudo
Q | A |
---|---|
Fixed Issues? | Fixes #10960 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | Yes |
Tests Added + Pass? | Yes |
Documentation PR Link | babel/website#2172 |
Any Dependency Changes? | |
License | MIT |
Reffered the code from StringLiteral function for Numeric Literals. What can we use here instead of |
@JLHwung Made the changes. But it is affecting other tests. PTAL. |
packages/babel-generator/test/fixtures/escapes/jsonEscape/output.js
Outdated
Show resolved
Hide resolved
packages/babel-generator/test/fixtures/minified/literals/output.js
Outdated
Show resolved
Hide resolved
packages/babel-generator/test/fixtures/escapes/jsonEscape/options.json
Outdated
Show resolved
Hide resolved
@JLHwung @nicolo-ribaudo PTAL now.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the logic should be something like this:
- Is
this.format.jsescOption.numbers
is set,
i. Callthis.number(jsesc(node.value, this.format.jsescOption))
- Otherwise, fallback to our old unmodified logic.
If someone sets this.format.jsescOption.numbers
, they are explicitly overwriting any other automatic logic used to format numbers (i.e. literal length when minified: true
, or "use the raw value if it's possible").
@nicolo-ribaudo @JLHwung PTAL now. Have modified the conditions as mentioned above. |
@nicolo-ribaudo Made the changes. PTAL now. |
@nicolo-ribaudo Done. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!