dapriett / nativescript-google-maps-sdk

Cross Platform Google Maps SDK for Nativescript
MIT License
244 stars 164 forks source link

Nativescript 7.x compatibility #420

Closed funder7 closed 3 years ago

funder7 commented 4 years ago

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

funder7 commented 4 years ago

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 commented 4 years ago

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

funder7 commented 4 years ago

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

flocca commented 4 years ago

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

funder7 commented 3 years ago

@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

funder7 commented 3 years ago

@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 ;)

dapriett commented 3 years ago

Thanks for all your work @funder7. Sorry I've been slow to respond, I'll take a look this week to get it merged. Let me know how the full test goes. Are there any updates needed for the demo apps?

funder7 commented 3 years ago

Hello @dapriett, nice to hear something from you! :-) I'm updating the demo projects now..it's taking more more time than I thought, but please before merging let's test this properly, otherwise who will download the plugin, will end with not working demo projects.

Unfortunately I can work on this only during the weekends, the changes that I pushed yesterday would fix the markers problem. Anyway I would wait until this release of nativescript reaches a more stable point, once the CLI gets updated, new problems are coming out. It's driving me to madness honestly!

Any help regarding the demo projects & documentation update is appreciated, in order to finish this work sooner.

liamcharmer commented 3 years ago

Heya just wondered if there is an ETA for release of this to get this working? Thank you so much for the work

funder7 commented 3 years ago

Sorry @liamcharmer I'm a bit busy in these days. In fact the code is ok, if you want you can clone this branch and build the plugin on your computer, then link it in your package.json (instructions here). It's not that hard, just go into the src folder, then npm run build, finally in the root there's a publish folder where you need to exec npm install && ./pack.sh. Now there will be a new publish/package folder, inside of it you can find the plugin pack that must be linked into your app package.json.

Hope this helps in the meanwhile that the work will be done.

liamcharmer commented 3 years ago

@funder7 Don't say sorry at all! Thank you so much for the amazing hard work! I'm a bit useless wit this type of stuff but your instructions have been perfect. I'll let you know what bugs i come across if i do come across any

liamcharmer commented 3 years ago

When I run exec pack.sh in the publish folder it does seem to crash the terminal for some strange reason. Will look into

funder7 commented 3 years ago

When I run exec pack.sh in the publish folder it does seem to crash the terminal for some strange reason. Will look into

Before you need to run npm install in that folder too! I forgot that :-)

Ok thanks for keeping an eye on it!

liamcharmer commented 3 years ago

When I run exec pack.sh in the publish folder it does seem to crash the terminal for some strange reason. Will look into

I was being silly i was running exec pack.sh not sh pack.sh

liamcharmer commented 3 years ago

@funder7

Seems to occur when running the app via ns run

(CoreFoundation) *** Terminating app due to uncaught exception 'NativeScript encountered a fatal error: Uncaught ReferenceError: NativeClass is not defined at ../node_modules/nativescript-google-maps-sdk/map-view.js(file: node_modules/nativescript-google-maps-sdk/map-view.ios.js:51:0)

funder7 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

funder7 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sal1n commented 3 years ago

Working for me too, thanks!

hypery2k commented 3 years ago

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

funder7 commented 3 years ago

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 commented 3 years ago

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

MrSnoozles commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

funder7 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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

funder7 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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...

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.

kefahB commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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