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

Nativescript 7.x compatibility #420

Closed
wants to merge 17 commits into from
Closed

Nativescript 7.x compatibility #420

wants to merge 17 commits into from

Conversation

funder7
Copy link

@funder7 funder7 commented Jul 20, 2020

Hi,

this PR updates the plugin by following the guidelines reported in this blog post, fixes the #422 issue (markers not showing), plus some other minor code improvements.

Cheers

- tns-core-modules is not required anymore, use @nativescript/core instead
- removed tns-core-modules typings (deprecated)
- momentarily added skipLibCheck: true
- version bump (beta)
- plugin build OK
- references.d.ts should use the new module
- fixed tsc compile errors without using @nativescript/core types
@funder7
Copy link
Author

funder7 commented Jul 22, 2020

Some updates: unfortunately I'm not in touch yet with NS team, new commits are including the fix for the problem reported in the last message.
There are still issues with WeakRef at runtime though, it needs further investigation.

@funder7 funder7 changed the title Feature/angular 10 compat Nativescript 6.x compatibility Jul 22, 2020
src/package.json Outdated Show resolved Hide resolved
src/tsconfig.json Outdated Show resolved Hide resolved
- WeakRef import not needed anymore with @nativescript/types global def
@funder7
Copy link
Author

funder7 commented Jul 22, 2020

Problem fixed! Successfully loaded the map in a nativescript v6 application! (tested on android only)

@funder7
Copy link
Author

funder7 commented Jul 23, 2020

I had an issue with markers not showing on the map. Apparently this is happening because the nativeView is cleared after initialization (disposeNativeView() method is called in the wrong moment?).

By looking at nativescript/core view management, seems that some changes have been introduced in the view lifecycle, so this needs to fixed before closing the PR!

In fact I started this migration due to this problem, people using the last released version will not see markers if they update to {N}6

- parseMultipleTemplates,parse are deprecated
- installed eslint with typescript-eslint
- basic configuration
- ignored files & folders
- new lint scripts
uses git hooks to avoid committing code with linting errors
@flocca
Copy link

flocca commented Aug 25, 2020

Sadly I can't show the markers even with this PR.

@funder7
Copy link
Author

funder7 commented Sep 2, 2020

@flocca this PR is not fixing the marker problem actually, it was more a generic update of dependencies and so on.

I thought that they were not showing due to the plugin dependencies not being up to date. In fact the source of the problem is in the code (nativescript is handling the view life-cycle differently, so the plugin is getting null instead of the map-view reference: it cannot load the markers).
The fix was not pushed because it wasn't fully tested, but now more than a month is passed without any problem, so if you can wait until this afternoon I'll push the update.

Cheers,
Federico

- avoiding to call GC on map elements (read in-code comment)
- imports condensed where possible
- fixed file with tslint recommendations
- optimized imports
- fixed files with tslint recommendations
- code formatting
- imports optimized
- ImageSource.fromResourceSync in place of imageSource.fromResource(deprecated)
- code formatting
follow the ns@7 update guidlines:
- update npm packages
- update ns plaforms to v7 (android & ios)
- add ts-node patcher and plugin config in tsconfig.json
- update tsconfig module config
- add ns global-types.d.ts support
- add @nativeclass() decorator where needed
- update prepare.js script version check

Ref #422
@funder7 funder7 changed the title Nativescript 6.x compatibility Nativescript 7.x compatibility Sep 6, 2020
@funder7
Copy link
Author

funder7 commented Sep 6, 2020

@flocca I've just pushed the fix, it includes nativescript@7 support, plus other stuff, now I'm closing so I didn't have time to test it in a project, btw the fix is well documented in map-view.android.ts in case you can import just that part in the previous version (b362ee0 commit).

Tomorrow I'll do a full test to check if everything is working properly.
In case that there are no problems, I will try to add the configuration for Angular 10&Ivy compiler compatibility.

Cheers ;)

- generate a new project with `ns create...`
- merge the existing project with the new one
@funder7
Copy link
Author

funder7 commented Sep 23, 2020

The stable version of this plugin is loading up or it's giving you the same exception?
If not, can you upload a blank ns-vue project with your configuration?

@liamcharmer
Copy link

@funder7 I've just created a new ns-vue blank project here: https://github.com/liamcharmer/nativescript-vue-plain-google-map-test

NPM Version 6.14.7
Node Version 14.8.0
NativeScript 7.0.8
iPhone iOS 14

@funder7
Copy link
Author

funder7 commented Sep 23, 2020

Ok mate, I will try to check it this weekend! Thanks for the project

@funder7
Copy link
Author

funder7 commented Sep 24, 2020

Another user reported that the MapView is not loading at all in this comment.
I guess that the problem reported by @liamcharmer affects all flavours, not only vue

@liamcharmer
Copy link

Ok mate, I will try to check it this weekend! Thanks for the project

Don't feel rushed or anything, you're honestly doing such amazing stuff and saving so many people. But keep good health!

@3rror404
Copy link

3rror404 commented Sep 27, 2020

Hi guys,

I am also seeing the NativeClass is not defined error using NS7, as well as a few others:

WARNING in ../node_modules/nativescript-google-maps-sdk/map-view-common.js 72:0-30
"export 'Style' (reexported as 'StyleBase') was not found in './map-view'
 @ ../node_modules/nativescript-google-maps-sdk/map-view.js
 @ ./create-incident/location-search.xml
 @ . sync (?<!\bApp_Resources\b.*)(?<!\.\/\btests\b\/.*?)\.(xml|css|js|(?<!\.d\.)ts|(?<!\b_[\w-]*\.)scss)$
 @ ./app.js

WARNING in ../node_modules/nativescript-google-maps-sdk/map-view.js 240:63-70
"export 'WeakRef' was not found in '@nativescript/core/debugger/dom-node'
 @ ./create-incident/location-search.xml
 @ . sync (?<!\bApp_Resources\b.*)(?<!\.\/\btests\b\/.*?)\.(xml|css|js|(?<!\.d\.)ts|(?<!\b_[\w-]*\.)scss)$
 @ ./app.js

WARNING in ../node_modules/nativescript-google-maps-sdk/map-view.js 241:75-82
"export 'WeakRef' was not found in '@nativescript/core/debugger/dom-node'
 @ ./create-incident/location-search.xml
 @ . sync (?<!\bApp_Resources\b.*)(?<!\.\/\btests\b\/.*?)\.(xml|css|js|(?<!\.d\.)ts|(?<!\b_[\w-]*\.)scss)$
 @ ./app.js

After a couple of hours of poking around I've finally got the app to build and the map to load without any warnings or crashes. These changes may have broken other things - I haven't had chance to test much of map functionality yet. Hopefully this might help somebody get up and running...


Terminating app due to uncaught exception 'NativeScript encountered a fatal error: Uncaught ReferenceError: NativeClass is not defined

I modified the src/tsconfig.json to be more similar to the official NS plugin tsconfig:

{
  "compilerOptions": {
    "target": "es2017",
    "module": "esnext",
    "moduleResolution": "node",
    "declaration": false,
    "noImplicitAny": false,
    "removeComments": true,
    "preserveConstEnums": true,
    "suppressImplicitAnyIndexErrors": true,
    "sourceMap": false,
    "pretty": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "noEmitHelpers": true,
    "noEmitOnError": true,
    "lib": [
      "es2017",
      "dom"
    ],
    "baseUrl": ".",
    "paths": {
      "*": [
        "./node_modules/*"
      ]
    },
    "plugins": [
      {
        "transform": "@nativescript/webpack/transformers/ns-transform-native-classes",
        "type": "raw"
      }
    ],
  },
  "include": [
    "./"
  ],
  "exclude": [
    "node_modules",
    "platforms"
  ]
}

publish/pack.sh seems to be loading the tsconfig.json file from the wrong directory:

Change this line:
node_modules/.bin/tsc -p "$TO_SOURCE_DIR/tsconfig.json"

To this:
node_modules/.bin/tsc -p "$SOURCE_DIR/tsconfig.json"


"export 'WeakRef' was not found in '@nativescript/core/debugger/dom-node'

I just removed this import from src/map-view.ios.ts:

import { WeakRef } from '@nativescript/core/debugger/dom-node';


"export 'Style' (reexported as 'StyleBase') was not found in './map-view'

Added an export of Style to src/map-view.ios.ts

export { StyleBase as Style };


To build the plugin:

  1. (This might not be necessary) In src/ run $ npm run setup
  2. $ npm run build
  3. In publish/ run $ ./pack.sh

To install the plugin:

  1. In your own app root $ tns plugin add /nativescript-google-maps-sdk/publish/package/nativescript-google-maps-sdk-3.0.0.tgz

@liamcharmer
Copy link

@3rror404 @funder7 can confirm the above has solved the issue for now and got the map working

@sal1n
Copy link

sal1n commented Oct 6, 2020

Working for me too, thanks!

@hypery2k
Copy link

hypery2k commented Oct 7, 2020

Just create a tgz (just download the file and remove .zip ;) ) for easier usage:
nativescript-google-maps-sdk-3.0.0.tgz.zip

@funder7
Copy link
Author

funder7 commented Oct 8, 2020

Hey @hypery2k nice work!
I don't know if that is possible but if you want open a new PR on my forked branch (feature/angular-10-compat) so we will include your work!

Regarding the pack.sh script....it creates a copy of the "src" directory, which if I don't remember bad is the "TO_SRC_DIR", then it does some operations on that folder, and finally it gets packed.

You can see what happens by commenting out the lines where the new directories are removed!

Cheers ;-)

@kefahB
Copy link
Contributor

kefahB commented Oct 21, 2020

Hi @funder7, What about the owner @dapriett ? he will merge this soon ?

@MrSnoozles
Copy link

Would be great to see the changes made by @funder7 and @hypery2k getting merged soon.
This plugin is just way too important to not be compatible with {N} 7. 😃

@funder7
Copy link
Author

funder7 commented Oct 22, 2020

Hello guys, I agree with you @MrSnoozles, before merging demo projects must be updated & tested, in order to provide to new users a good starting point. Unfortunately I'm really busy in these days and it's hard to find a moment to finish the upgrade.
Another point is iOS testing: I don't own an iPhone so it's impossible for me, I should setup a remote testing configuration somehow.
I'll do my best to find a spot in the next weeks, but I cannot guarantee it...If there's a candidate wanting to finish this tasks, I can give some support in case he/she needs it.
;-)

@rljdavies
Copy link

I've been working on this (along with others of course) and will do everything I can. At the moment apart from the demos, there seems to be issues with all events on IOS apart from mapReady. Ive seen at least one other commenting similar.

I can run extensive tests on IOS both physical and via simulators and Im not seeing anything other than mapReady firing. I'm thinking the new @nativeclass requirement is the issue.

@kefahB
Copy link
Contributor

kefahB commented Oct 23, 2020

@funder7 I have can do the job .. can you do a briefing about the status of jobs ?

@funder7
Copy link
Author

funder7 commented Oct 24, 2020

Hi guys, thanks for your interest.
@rljdavies I think that you're right. I'm pretty sure to have read a post on NS blog, telling to add the @NativeClass() decorator on each plugin class extending a iOS native object.
An example:

@NativeClass()
class IndoorDisplayDelegateImpl extends NSObject implements GMSIndoorDisplayDelegate {

Would that be sufficient? I don't know, but I hope so. I just looked at some code that I've commited in the past months and it seems that decorators are already in place.

But speaking with another plugin user, he told me about a memory leak problem that he noticed. When unloading the view which contains the map, memory is not freed. So if you continue to do back and forth from the map view, a new instance is created every time, but not deleted once you leave.
I'm almost sure that a breaking change has been introduced in the view lifecycle in the latest versions, so the plugin must be adapted in order to load(and unload) correctly. I've made a temporary fix for issue #422 , which is touching the area where I suspect this problem resides, you can see the updated code here, check disposeNativeView().

You can try to do a debug session and check that in the iOS part, all references are populated correctly, it's very likely that something is not initialized and is null, that's why event listeners may be not hooked correctly. Just an assumption.

@kefahB demo projects are carried with the plugin to provide a minimal working configuration for each NS flavour (Angular, Vue.js, Vanilla JS, etc.. ). So there are some changes to be done:

  • Angular demo project must be updated to Angular 10.x,
  • The way how mapView is initialized in the Angular project must be updated to follow latest guidelines for @ViewChild usage
  • Update all the projects dependencies if possibile (maybe comparing the project with a more recent one)
  • Update the Readme to include the new updates

Generally speaking that's what's missing, solving iOS issues and checking the mapView lifecycle on both platforms is a top priority task though, maybe let's focus on that before completing the rest.

We can organize a day where we "meet" and try to fix all this problem, to close this update. Let me know what do you think about it, generally I'm free on saturday or sunday (doh)

Cheers guys

@rljdavies
Copy link

Thanks for the comprehensive post @funder7

Would that be sufficient? I don't know, but I hope so. I just looked at some code that I've commited in the past months and it seems that decorators are already in place.

I think/hope so too. I've seen the decorator in some places but not others I might have expected them to be.

I have to push an App Store update this evening with partial fixes in a client app, but once that is out I can turn back to this.

@kefahB
Copy link
Contributor

kefahB commented Oct 24, 2020

@funder7 It will be great if you push your changes to your fork, then I will continue from there

@funder7
Copy link
Author

funder7 commented Oct 25, 2020

@rljdavies no problem, we can also arrange this for the next weekend

@kefahB Ok I'll push the changes in a separate branch, as more than a month passed and now I don't remember if what I was doing was kind of a test, or definitive code. They're more than 200 files :(
Since there are some changes on plugin code, I'll push them in a separated commit, please keep an eye on them too.

I'll post the branch name once everything is pushed

@funder7
Copy link
Author

funder7 commented Oct 25, 2020

Ok... so I pushed my changes to refactor/demo-projects-update, but I have bad news: files were marked as changed but in fact only their file permissions were changed, don't ask me why. On 200+ changes, only few of them were really updated, and I can tell that those updates can be trashed straight away, sorry guys.

If you want, we can meet next saturday afternoon and try to finish this update. Let me know if you're available. Cheers

@kefahB
Copy link
Contributor

kefahB commented Oct 25, 2020

@funder7 I've forked the original project .. and successful running on IOS, there is only one issue, infoWindowTemplates dose not work! I’ll keep this for the moment try with android then we will see

@funder7
Copy link
Author

funder7 commented Oct 26, 2020

Nice! So #422 happens on android only, or recent ns updates have solved the problem.

The current version has a typo in the class where map styling json is parsed, in case you need that function give it a check.

If you can, keep an eye on memory usage, in particular I'd like to know if memory is freed once the map view is unloaded.

It's kinda weird though that it works with the old configuration, but not after linking new nativescript libraries. I will try to compare the transpiled output of both version just to understand what differs.

Thanks for reporting!

@kefahB
Copy link
Contributor

kefahB commented Oct 26, 2020

I run the project on both platforms successfully without problem .. I'll try to debug the memory for android...

Also I have implemented CLLocationManagerDelegate to get the button myLocationButton worked .. it have never worked on IOS :)

@rljdavies
Copy link

I found a reply from TSC to a question you asked a little while back @funder7 - confirming NativeClass decorator is not needed in the typings files. So here's what I did...

  • Rebuilt my project from scratch, not just ns clean, a completely new start beginning with ns create.
  • Followed steps outlined by @3rror404 (except the StyleBase step as this for me created a circular reference).
  • Rather than overwriting plugin files I removed the plugin fully and installed from the local tgz as mentioned by @3rror404

After I did the above I was able to run my project including with map events (eg MapMarkerSelected) firing on IOS.
However, native classes (eg GMSCameraPosition) were not found.

So, I copied the typings files in the plugin's platforms/ios folder to the @nativescript/types-ios/lib/ios/objc-x86_64 folder and added references to them in @nativescript/types-ios/lib/ios/ios.d.ts and everything I'm using plugin wise now appears to be working (although one issue with preparing a release build remains)

Therefore, there does seem to be an issue with references but this point I'm unsure if this is specific to this plugin or something more general with NS7. Its worth pointing out not all of the plugins in the project are NS7 but in those cases they are minor plugins with limited function.

Hopefully this info moves us on a little.

@AgustinV08 AgustinV08 mentioned this pull request Oct 29, 2020
@kefahB
Copy link
Contributor

kefahB commented Oct 29, 2020

Hi @funder7 ,

I've pushed a new PR .. I tested on multiples device and it work "properly" but the infoWindowTemplates does't work for the moment !

@funder7
Copy link
Author

funder7 commented Nov 10, 2020

Nice work guys! I'm sorry about not being able to contribute, but I'm very busy.

I think that creating a native app on the respective platform, including Google Maps (the same version used by this plugin) would highlight any kind of problem related to component initialization & configuration.

Also I don't know if it's possible to debug the full application, from the Java/Objective-C layer to the Javascript code. That probably must be done by doing one layer at time. But probably this is the last resort.

@rljdavies Thanks for the info about decorators and typings. Btw the map is visible and can be panned, so in order to troubleshoot the problem related to camera changed event, I would split the investigation like this (always if anybody has the time to do it!):

  1. Find if the native maps component is firing the event
  2. Check if the view(or any other component) where the map is rendered, is able to see that event
  3. If the previous points are OK, then the only point of failure for me is the plugin not being able to detect the event, maybe due to some kind of in-between layer added with NS7 that changed how the native view and the javascript code are able to communicate to each other.

There's another point regarding the infoWindowTemplate mentioned by @kefahB thay may benefit from a check-up on the view rendering & link between the layers.

Debugging here can be really helpful, by placing a breakpoint where the infoWindowTemplate is fired it would be possible to see if everything that it needs to show up is in place. At least this is how I've fixed the markers problem.

Markers were not showing because with NS7, some plugin code was running in the wrong moment. It was code used to avoid memory leaks I think, and it was called originally on view destroy. With NS7 that was called during the view initialization, and if my memory works, it was called twice during the navigation from a view to another one.

Long speech here :-) ... btw to sum up, a full debugging in every step of the plugin's view lifecycle, would help to understand what happening. If we only could know what changed into the "magic black box"....... :-D

@dapriett
Copy link
Owner

dapriett commented Nov 13, 2020

Closing in favor of #440 - nice work @funder7 and @kefahB!

@dapriett dapriett closed this Nov 13, 2020
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.

10 participants