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

spread operator compatibility #304

Closed
JuniorTour opened this issue Jun 14, 2020 · 9 comments · Fixed by #311
Closed

spread operator compatibility #304

JuniorTour opened this issue Jun 14, 2020 · 9 comments · Fixed by #311

Comments

@JuniorTour
Copy link

Version

4.0.0-alpha.12

Reproduction link

https://cdn.bootcdn.net/ajax/libs/vue-router/4.0.0-alpha.12/vue-router.global.js

Steps to reproduce

<link> and execute the cdn script in Edge@44.

What is expected?

Everything works fine.

What is actually happening?

Page blank, and got console error: SCRIPT1028: SCRIPT1028: Expected identifier, string or number vue-router.global.js (374,15)


May be we need a polyfiill function for spread operator?

@danyadev
Copy link

Vue uses Object.assign to fix this problem. Using polyfill will add some extra code, which can simply be avoided.

posva added a commit that referenced this issue Jun 16, 2020
posva added a commit that referenced this issue Jun 17, 2020
posva added a commit that referenced this issue Jun 18, 2020
* fix: use assign to align with Vue browser support

Close #304

* fix: use correct imports

* fix: more missed objects

* fix: add missing copy
@BenoitRanque
Copy link
Contributor

BenoitRanque commented Aug 24, 2020

I have run into this problem again. Should I open a new issue?

I have identified the offending lines of code, use of spread operator here and here

There is also a dangling comma here

Manually removing those from the dist. file fixes the issue for me. I am able to create a PR if need be

@posva
Copy link
Member

posva commented Aug 24, 2020

@BenoitRanque Thanks, I removed the spread at 71c28cf. The warning is a bit different, I'm open to proposals to keep it correct in TS and yield a version that works on older Edge versions. Keep in mind, it's removed in production builds, so it should still run fine on production. If it doesn't, there there is something wrong, so yes, please open a new issue!
The dangling comma is removed when bundled. I see it know, that looks like a bug somewhere in the bundling chain. Will take a look

@BenoitRanque
Copy link
Contributor

@BenoitRanque Thanks, I removed the spread at 71c28cf. The warning is a bit different, I'm open to proposals to keep it correct in TS and yield a version that works on older Edge versions
The dangling comma is removed when bundled

The dangling coma is not removed, which is how I found it. Search for ,] in the CDN

https://cdn.bootcdn.net/ajax/libs/vue-router/4.0.0-beta.7/vue-router.global.js

@BenoitRanque
Copy link
Contributor

BenoitRanque commented Aug 24, 2020

About the warn method, it is indeed tricky. For now I've got this, but the unused argument is icky, and I still need to type it. Will update asap

function warn (msg: string) {
	console.warn.apply(console, ['[Vue Router warn]: '].concat(arguments))
}

Edit: while this should work on the JS side of things, typescript will probably complain if more than one argument is passed...

Ouch

@posva
Copy link
Member

posva commented Aug 24, 2020

The dangling comma is fixed at f7ccde0

@BenoitRanque
Copy link
Contributor

BenoitRanque commented Aug 24, 2020

Update: with help on the discord, came up with this:

export function warn(msg: string, ...arg: any[]): void
export function warn(msg: string) {
  let args = Array.from(arguments).slice(1)
  console.warn.apply(
    console,
    ['[Vue Router warn]: ' + msg].concat(args) as [string, ...any[]]
  )
}

Which, on build, gives this output:

function warn(msg) {
    let args = Array.from(arguments).slice(1);
    console.warn.apply(console, ['[Vue Router warn]: ' + msg].concat(args));
}

This should work, but it will need some comments explaining why

If this looks good to you, I can create a PR

Edit: This does not trigger and TS error on dev or build, should have specified.
I've also tested the resulting function with warn('string') and warn('string', object) and it seems to work as expected

@posva
Copy link
Member

posva commented Aug 24, 2020

Nice! I had a similar solution but I like yours more and it works better too.
I added you as a co author at cf1fda3

@BenoitRanque
Copy link
Contributor

Awesome! I've just tested it in my project (which uses webview) and it works as expected.
I'm using a locally built version of the file as a dependency until this goes live on npm.

Thanks a bunch!

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 a pull request may close this issue.

4 participants