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

fix(compiler): Allow BigInt usage in templates #11152

Merged
merged 8 commits into from Mar 30, 2021
Merged

Conversation

shadowings-zy
Copy link
Contributor

@shadowings-zy shadowings-zy commented Feb 27, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • [ x ] Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • [ x ] No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • [ x ] A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Hello!
This pull request is related to issue #11126 . Now we can use BigInt in "Mustache" syntax.
I had already added test for this feature in "test/unit/features/filter/filter.spec.js".
And all tests are passing.

Close #11126

@posva
Copy link
Member

posva commented Feb 27, 2020

Hi, thanks for the PR but this is not what was mentioned on the issue. The idea is to allow the use of BigInt when it's available and fail if it's not. See https://jsfiddle.net/syrnaefq/. This should work on browsers supporting BigInt. It's similar to allowing Number in templates. The syntax 100n seems to already work when it's supported by the browser.
The goal is not to polyfill the feature

@shadowings-zy
Copy link
Contributor Author

shadowings-zy commented Feb 27, 2020

That makes me a little confused...
In my pull request," {{ BigInt(100) }}" and "{{ 100n }}" both work when BigInt is available, and fail if it is not ( and my PR will warn 'BigInt is not support! ' )

It seems that the idea of this issue is the same to my PR🤦‍♂️?
@posva

see this test:

  it('bigint support', () => {
    const vm = new Vue({
      template: `<div>{{ BigInt(BigInt(10000000)) + BigInt(2000000000n) * 3000000n }}</div>`
    }).$mount()
    expect(vm.$el.textContent).toBe('6000000010000000')

@shadowings-zy
Copy link
Contributor Author

Did you mean: 'We need to "{{ BigInt(100) }}" and "{{ 100n }}" both work when BigInt is available, and fail if it is not' like ' "{{ Number(100) }}" and "{{ 100n }}" both work'?

If so, I get your point and I will fix it in this way.

@shadowings-zy
Copy link
Contributor Author

Hi @posva !
I revert the previous commits and I rewrite the code to allow BigInt usage in templates.
As you see it just like allow Number in template.

截屏2020-02-2721 47 18

If it's OK, please merge this PR, if not, let's discuss further : )

const allowedGlobals = makeMap(
'Infinity,undefined,NaN,isFinite,isNaN,' +
'parseFloat,parseInt,decodeURI,decodeURIComponent,encodeURI,encodeURIComponent,' +
'Math,Number,Date,Array,Object,Boolean,String,RegExp,Map,Set,JSON,Intl,' +
'require' // for Webpack/Browserify
'require,' + // for Webpack/Browserify
(supportBigInt ? 'BigInt' : '') // for BigInt support issue #11126
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to not use a conditional, the same way we allow Intl even though it's not supported before IE 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,That sounds right,and I will add BigInt directly in makeMap()
: )

@@ -10,7 +10,8 @@ if (process.env.NODE_ENV !== 'production') {
'Infinity,undefined,NaN,isFinite,isNaN,' +
'parseFloat,parseInt,decodeURI,decodeURIComponent,encodeURI,encodeURIComponent,' +
'Math,Number,Date,Array,Object,Boolean,String,RegExp,Map,Set,JSON,Intl,' +
'require' // for Webpack/Browserify
'require,' + // for Webpack/Browserify
'BigInt' // for BigInt support issue #11126
Copy link
Member

Choose a reason for hiding this comment

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

let's put it up after the Intl, the comment is not really necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK~, I fixed it in commit#4241f12

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

@shadowings-zy can you undo the merge you did? it seems to have messed up the PR

@shadowings-zy
Copy link
Contributor Author

@shadowings-zy can you undo the merge you did? it seems to have messed up the PR

@posva Sorry, I forgot I use dev branch to open this PR, and now I undo my merge.

@posva posva changed the title feat(compiler): Allow BigInt usage in templates (issue #11126) feat(compiler): Allow BigInt usage in templates Nov 13, 2020
@posva posva added this to In Review in 2.6.13 Feb 24, 2021
@posva posva moved this from In Review to Reviewed once, needs another review in 2.6.13 Mar 9, 2021
@posva posva changed the title feat(compiler): Allow BigInt usage in templates fix(compiler): Allow BigInt usage in templates Mar 30, 2021
@posva posva merged commit c42b706 into vuejs:dev Mar 30, 2021
@posva posva moved this from Reviewed once, needs another review to Done in 2.6.13 Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Allow BigInt usage in templates
2 participants