Skip to content

Conversation

@EddyVerbruggen
Copy link

Hi,

I was checking out the latest seed code today and I was 😃 to find that the linking and watching works REALLY WELL these days!

I stumbled upon a few things that I'd like to see corrected, so here's a PR for these thingies:

  • Users were forced to use the tns-core-modules/ prefix when importing core modules in the plugin and demo code. I don't think that's what real apps do so we shouldn't make it different for plugin authors IMO.
  • Fixed a few typo's in the readme.
  • Cleaned up .gitignore so it no longer adds <plugin>.d.ts files to GitHub (those are only required for npm, and are generated by tsc locally when changing plugin code). Also removed a few redundant lines.
  • When running tns run ios in the demo folder the TypeScript compiler complained about native references (android.). These could be ignored but it may confuse people, so I've added the platform declarations to the demo as well.
  • Changed rm -rf to rimraf in package.json for Windows compatibility.

Feel free to squash these commits as I was a bit commit-happy :)

@ghost ghost assigned EddyVerbruggen Aug 3, 2017
@ghost ghost added the new PR label Aug 3, 2017
"baseUrl": ".",
"paths": {
"*": [
"./node_modules/tns-core-modules/*",
Copy link

Choose a reason for hiding this comment

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

I suggest not use use paths to the core modules but write the full imports (import { .. } from "tns-core-modules/ui/view") because of issues with the Angular AoT compiler.
angular/angular-cli#5618

Copy link
Author

Choose a reason for hiding this comment

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

AFAIK that's fixed by adding skipLibCheck: true to tsconfig.json (what this PR does also). I had that same issue and adding that property made webpack behave nicely.

Copy link

Choose a reason for hiding this comment

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

I think that skipLibCheck resolves another problem which happen to fail with the same error (heap out of memory).

Copy link
Author

Choose a reason for hiding this comment

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

That's true. But is the recommendation for all NativeScript users that want to use AoT to prefix their modules (in both apps and plugins)? I don't see that happening in the Groceries sample for instance so this confuses me.

Is there a way to reproduce the problem with my suggested changes?

Copy link

Choose a reason for hiding this comment

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

The AoT compilation in Groceries (as well as the other NS apps, maintained by the team) is used only with webpack. When bundling with webpack, we use the tsconfig.aot.json configuration for TypeScript. The paths, specified in the tsconfig.aot.json, are different from the ones in tsconfig.json. I'll call them full paths from now on. Using the full paths helps prevent the memory problem when bundling. I wrote more about that in the already mentioned issue - angular/angular-cli#5618 (comment).
You may ask why don't we use the same approach for plugins. I think that adding the full tsconfig paths in all plugins is not a sustainable solution. After all, we may add new folders to tns-core-modules, change the names of the existing ones or even drop some of them. If that happens, the plugin authors will have to go over all their plugins' tsconfigs and update them one by one or they won't be able to compile them with tsc anymore.
But what about the apps that have the full paths? Well, as I mentioned, they are placed in the tsconfig.aot.json file - and this one is added by the nativescript-dev-webpack plugin. Then, everyone that has that file, uses the plugin. If a change is required in the full paths, we can use the ns-webpack to modify the tsconfig.aot.json file (on postinstall, for example) and fix the user's project.
After all, we hope that the memory problem when using @ngtools/webpack gets resolved soon. But before that happens, I think it's best to recommend for plugin authors to use full imports - tns-core-modules/....

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean. I'm just wondering if you webpack an app that doesn't use full paths (but has a tsconfig.aot.json file): what difference would it make if the plugin uses full paths or not? I'd assume the paths are resolved by webpack similarly as the app's code.

Copy link

Choose a reason for hiding this comment

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

The full imports will get resolved faster during build time, but the difference will be negligible unless there are hundreds of thousands imports from tns-core-modules. It's just because TS will first try to find the import as-is and then go through each of the path mappings.

Copy link
Author

Choose a reason for hiding this comment

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

Alrighty! Thanks for taking the time to explain the reasoning to me, much appreciated. I've reverted the relevant bits.

@radeva radeva merged commit fda6f60 into NativeScript:master Aug 4, 2017
@ghost ghost removed the new PR label Aug 4, 2017
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.

4 participants