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

support for Vue 2.6 #445

Closed
wants to merge 17 commits into from
Closed

Conversation

farfromrefug
Copy link
Contributor

No description provided.

@tralves
Copy link
Contributor

tralves commented Mar 6, 2019

Hi @farfromrefug! Awesome work! This PR looks ok and I need 2.6 in my project, but there are lots of changes here.
Can you please describe in more detail what you did and why? I think it will help @rigor789 review and accept it.

@rigor789, please review and accept it 😛 I am going to test this on my projects to see if I find any issues.

@farfromrefug
Copy link
Contributor Author

Basically i did 3 things:

  • fixed the build process. It was broken and the nativescript-vue-template-compiler was not building anymore. Mostly this was due to registerElement being needed within webpack. Because of that when doing require('nativescript-vue-template-compiler') within your webpack config you were trying to import tns-core-modules which can't work. So i had to refactor things a bit and create a different elementRegistry for webpack/runtime (though they can actually be merge, i know how to do it).
  • update the runtime to work with vue 2.6. It broke a few things within the nativescript-vue and i am not even sure i have seen them all yet. For now i saw that the view directive and the listview patchTemplate were broken.
  • remove the console.log override as it was creating crash within my apps with complex object logging

Already using that branch within my project without error

@rigor789
Copy link
Member

rigor789 commented Mar 6, 2019

@tralves @farfromrefug it's on my todo list to go over this as soon as I can - but I can't promise anything...

Copy link
Member

@rigor789 rigor789 left a comment

Choose a reason for hiding this comment

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

I've left a few comments that I'd like to know more about before merging.

@farfromrefug you said you can refactor the element-registry to be unified - can you please do that?
With this PR, we'd have one element-register and two element-registry's which seems unnecessary and likely to cause issues in the future, as both need to be updated simultaneously.

build/config.js Outdated Show resolved Hide resolved
build/config.js Outdated
@@ -17,6 +17,12 @@ const banner = (name, version) => `
* Released under the MIT license.
*/
`
const intro = `
if (!this['process']) {
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug any reason you are not checking for global.process? In this case I believe it does the same thing, since we are in a global context - but still curious if there is something I'm not aware of! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well to be honest i am not sure how "process" is handled in Nativescript. What i know is that VueJs checks process and not global.process. That's why i used this.

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug process is not defined in NativeScript by default, and the reason we're adding it manually is that some vue plugins check for process.env which then causes issues since it's trying to access property env of undefined. Checking if it exists on global and then adding to it ensures that both global.process and just process are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 VueJs 2.6 also uses it. So you are saying that i should do?:

if (!global['process']) {
  global.process = process = {env:{}}
}

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug yeah that looks good to me, just change this['process'] to global['process']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, testing global.process

@@ -72,10 +81,9 @@ const genConfig = (name) => {
replace({
__WEEX__: false,
__VERSION__: VueVersion,
'process.env.NODE_ENV': "'development'",
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug Does this remove all the development related help messages that vue prints? If so - can this be re-enabled while developing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it s the user choice by replacing within his weback config. that code was making the appear always, even in prod

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug alright, so as far as I understand this change, this should still allow the help messages to appear unless webpack sets the env to "production"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 indeed it will appear by default because it checks for NODE_ENV !== 'production' which is the case as NODE_ENV is not defined.
However that change allow the user tu define NODE_ENV to production when building for production, and thus remove all the warning.
It is just moved to userland

// const expression = parseText(staticClass, options.delimiters)
// if (expression) {
// warn(
// `class="${staticClass}": ` +
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug any reason this has been removed? Interpolation in attributes should not be used - and should be warned about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it was a deprecation warning that was implying a regexp test on every transform node. Seemed like it was time to remove it. But i can bring it back if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug Just checked and it is still present in vue (web), so let's keep it there until even vue removes it! :)

https://github.com/vuejs/vue/blob/ebc1893faccd1a9d953a8e8feddcb49cf1b9004d/src/platforms/web/compiler/modules/class.js#L16-L22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

getAndRemoveAttr(el, attr)
addDirective(el, 'view', `v-view:${attrName}`, '', arg, modifiers)
getAndRemoveAttr(el, attr, true)
addDirective(el, 'view', `v-view:${attrName}`, '', false, arg, modifiers)
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug what do the added args do?

The first one is removeFromMap - why is this required?

The second one seems to be mapping to arg - is this the intended parameter? Or should that be isDynamicArg instead?

export function addDirective (
  el: ASTElement,
  name: string,
  rawName: string,
  value: string,
  arg: ?string,
  isDynamicArg: boolean,
  modifiers: ?ASTModifiers,
  range?: Range
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my bad i inversed the args. Maybe that's why arg was not working anymore! will fix

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug perfect, hopefully that will fix it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const newLineRegExp = /\\n/g

console.log = (function(log, inspect, Vue) {
return function(...args) {
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug I'm not too keen on removing this - I'd rather make it opt-out, as I've had a much better development experience with this in place vs. the default console.log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 ok i understand. Make it an opt-in then. GDPR ;) It still crashes for me + i don't really want it. And users won't understand why. If you can't recall why it was necessary then i think it really should be opt-in and not opt-out

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug the reason I had to add it was that the built-in console.log crashed the app for anything more "complex" than a string, or a simple object { foo: "bar" }. Most notably it crashed whenever trying to log a vue observed property, which was probably 90% of the things you'd log.

Plus it adds colors - which I really like when looking at the logs, makes it a lot easier to find things (this might be just my preference, and it's disabled by default on the playground anyways)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 those crashes do not appear anymore. Plus the contrary happens. Crashes with it.
If you use it just for the colors, should be in userland. plus it adds unecessary dependencies in production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 so we can keep this in userland?

platform/nativescript/framework.js Show resolved Hide resolved
const parent = el.parentNode.nativeView

const arg = binding.rawName.split(":")[1];
const modifiers = binding.modifiers;
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug I'm assuming this is a related change to platform/nativescript/compiler/modules/view.js, can you elaborate why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems to have changed in Vue 2.6, your was not working anymore and i could not get the "arg" as before

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug I'm assuming it's this commit that changed directives: vuejs/vue@dbc0582

wonder if changing the order in platform/nativescript/compiler/modules/view.js for the arguments would make it work without this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix working as we talked about

import { makeMap, once } from 'shared/util'
import { VUE_VM_REF } from '../runtime'
const VUE_VM_REF = '__vue_vm_ref__'
Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug Why do we need to re-declare rather than importing? Is this due to the fact that runtime/index relies on tns-core-modules and should not be referenced to avoid build issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rigor789 yes indeed. May be we should have a constants.ts with the constant define ?

Copy link
Member

Choose a reason for hiding this comment

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

@farfromrefug yes, I like the idea of a constants file - should keep the codebase tidier!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tralves
Copy link
Contributor

tralves commented Apr 8, 2019

I think I found another issue.
This branch doesn't seem to support the new scope slot syntax v-slot introduced in 2.6...

I get this: [Vue warn]: Failed to resolve directive: slot
I thought maybe NS-Vue is doing something weird with directives?...

@farfromrefug
Copy link
Contributor Author

I think I found another issue.
This branch doesn't seem to support the new scope slot syntax v-slot introduced in 2.6...

I get this: [Vue warn]: Failed to resolve directive: slot
I thought maybe NS-Vue is doing something weird with directives?...

Is is what you use with nativescript-multi-drawer ? Cause if it is, it's working for me. Can you share an example project?

@tralves
Copy link
Contributor

tralves commented Apr 9, 2019

This is the example project: https://github.com/tralves/ns-vue-v-slot-test

This is the relevant part in App.Vue:

<StackLayout>
  <Label class="message" text="OLD SYNTAX (slot-scope)" col="0" row="0"/>
  <CurrentUser>
    <template slot-scope="scope">{{scope.user.firstName}} {{scope.user.lastName}}</template>
  </CurrentUser>

  <Label class="message" text="NEW SYNTAX (v-slot)" col="0" row="0"/>
  <CurrentUser>
    <template v-slot="scope">{{ scope ? scope.user.firstName : '(scope is not defined)'}}</template>
  </CurrentUser>
</StackLayout>

The old syntax slot-scope works, but the new syntax v-slot doesn't (see the docs).

That warn [Vue warn]: Failed to resolve directive: slot is not showing anymore. I think I was not using it with <template>...

@farfromrefug
Copy link
Contributor Author

@tralves i see. Don't use that feature yet. Think it should be good to merge that PR first. Then implement that new feature. @rigor789 any news on this?
I need to know if you are about to merge this. Otherwise i will make my fork public under another name.

Thanks

@rigor789
Copy link
Member

@farfromrefug I will try to go over it in the next few days!

@keithgulbro
Copy link

Hi @tralves,

Any update on the use of v-slot? I'm attempting to use named slots and still encountering an issue, I think this feature is of significant value given VueJS is designed to use component tree's.

@tralves
Copy link
Contributor

tralves commented Apr 24, 2019

@keithgulbro no news here. I tried to find the cause but couldn't figure it out. But you can still use the deprecated slot syntax. That one works.

@farfromrefug
Copy link
Contributor Author

farfromrefug commented Apr 25, 2019

@tralves think i just found why it is not working with the new syntax.
Will try and fix it.
But i think it will be in a fork.

EDIT: @tralves can you try with akylas-nativescript-vue at version 2.1.14? It might work
You might need to replace the compiler too akylas-nativescript-vue-template-compiler

@rigor789
Copy link
Member

Changes landed in #487 and v2.3.0-rc.0

@rigor789 rigor789 closed this May 31, 2019
@farfromrefug
Copy link
Contributor Author

@rigor789 will try your rc. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants