search

nstudio / nativescript-loading-indicator

NativeScript Loading Indicator plugin
Apache License 2.0
49 stars 14 forks source link

Change CommonOptions to abstract the nativeView instead of platform options. #1

Open rkJun opened 5 years ago

rkJun commented 5 years ago

Which platform(s) does your issue occur on?

It occurs when tns run ios or tns run android

Please, provide the following version numbers that your issue occurs with:

  "dependencies": {
    "@nstudio/nativescript-loading-indicator": "^1.0.0",
    "nativescript-background-http": "^3.4.0",
    "nativescript-cardview": "^3.1.1",
    "nativescript-carousel": "^4.1.0",
    "nativescript-checkbox": "^3.0.3",
    "nativescript-facebook": "^3.0.1",
    "nativescript-floatingactionbutton": "^5.0.3",
    "nativescript-image-cache-it": "^5.0.0-alpha.3",
    "nativescript-imagepicker": "^6.2.0",
    "nativescript-oauth2": "^1.4.3",
    "nativescript-pager": "^9.5.4",
    "nativescript-plugin-firebase": "^8.3.2",
    "nativescript-purchase": "^2.0.12",
    "nativescript-theme-core": "~1.0.4",
    "nativescript-toasty": "^1.3.1",
    "nativescript-ui-listview": "^6.3.1",
    "nativescript-windowed-modal": "^5.0.6",
    "tns-core-modules": "^5.4.3",
    "uglifyjs-webpack-plugin": "^2.1.3",
    "validator": "^11.0.0"
  },
  "devDependencies": {
    "nativescript-dev-typescript": "^0.9.0",
    "nativescript-dev-webpack": "^0.24.1"
  },

Please, tell us how to recreate the issue in as much detail as possible.

Describe the steps to reproduce it.

I removed the android.view and ios.view from the options.

const options: OptionsCommon = {
...
    android: {
        // view: someStackLayout.android, // Target view to show on top of (Defaults to entire window)
        cancelable: true,
        cancelListener: function(dialog) {
            console.log('Loading cancelled');
        }
    },
    ios: {
        // view: someButton.ios, // Target view to show on top of (Defaults to entire window)
        square: false
...

It works. But, I have a build error log. When i tns run ios or tns run android in CLI, i get the error message. but it works.. is this ok?

    + 299 hidden modules

ERROR in /Users/.../dev/gitlab/app_name/node_modules/@nstudio/nativescript-loading-indicator/index.d.ts
ERROR in /Users/.../dev/gitlab/app_name/node_modules/@nstudio/nativescript-loading-indicator/index.d.ts(77,10):
TS2304: Cannot find name 'UIView'.

ERROR in /Users/.../dev/gitlab/app_name/node_modules/@nstudio/nativescript-loading-indicator/index.d.ts
ERROR in /Users/.../dev/gitlab/app_name/node_modules/@nstudio/nativescript-loading-indicator/index.d.ts(85,10):
TS2503: Cannot find namespace 'android'.
Webpack compilation complete. Watching for file changes.
Webpack build done!
nativescript-plugin-firebase: building for same environment, not forcing prepare.
Preparing project...
Project successfully prepared (Android)
Configure firebase
Successfully transferred all files on device 06157df6e2a....
Restarting application on device 06157df6e2a......
bradmartin commented 5 years ago

Your project shouldn't be analyzing node_modules

https://www.typescriptlang.org/docs/handbook/compiler-options.html search for -skipLibCheck. You can also look at the demo project here for how to set it in the tsconfig.

Reopen if that doesn't solve, but it should.

nikoTM commented 5 years ago

@bradmartin skipLibCheck works, but that is not the default configuration that is generated with nativescript-cli, meaning that if a user creates a new project and adds this plugin the build will fail for no obvious reasons. This solution should probably be mentioned in the readme, but ideally maybe the plugin could use View and extract native view from there, so that there is no dependency on platform declarations?

bradmartin commented 5 years ago

I'm in favor of them changing the templates, I can't see any reason why they aren't already doing that to be honest. It improves tsc performance drastically in most projects with many deps and files to process.

I also lean on the plugin being able to use the platform-declarations that NS provides, so I'll add a note to the README for now and see if they'll update their templates before making changes to the plugin.

Although your idea of grabbing the nativeView is a good idea 👍 so I'll reopen as a future improvement, but that will end up a breaking change if we remove the option, or we have to do a check for view in the common options and then the platform options which adds more code to maintain, so that's a bit discouraging for small OSS projects. Good idea though so thanks for that.