SilverMira / angular-snowpack-plugin

8 stars 0 forks source link

Feedback on the plugin #11

Open timofei-iatsenko opened 3 years ago

timofei-iatsenko commented 3 years ago

Hey @SilverMira just tried to setup snowpack with your plugin to our existing angular application (quite big codebase, with some custom tooling and setup)

First of all really thanks for your job. Angular platform is quite conservative and probably we never get oficial support of tool like snowpack or esbuild from angular team. So your job is quite apreciated because it's a good start to prove that angular can be build with modern frontend tools.

I worked with AngularCLI internals quite for a while and probably can help you with the development of this plugin.

Here is my thoughts on that:

First of all, for now I see snowpack just as a replacement for ng serve command. Because building angular for production requires much more advanced tooling such as build optimizer, tree shaking, i18n strings inlining, conditional bundles loading and more things what angular cli incapsulated and abstracted for end user. There really too much effort need to be done to at least partially implement what angular cli already has.

So i would like to concentrate more on improving development experience using snowpack.

The next thought which follows the first is, it would be nice if we this plugin would exist as custom angular cli builder which would be a drop-in replacement for @angular-devkit/build-angular:dev-server builder.

And for the majority of users, it would be as simple as just replace one builder to another in thiers angular.json. Or even provide a schematic for automated installation.

Regarding integration this into our projects. I fixed few issues but there are still errors and project doesn't build successfully:

angular.json schema inconsistency

Plugin hardoded to accept angular.json is most "standard" format. this caused errors in our build. Attaching a diff what i changed to make it work:

image

Type imports issues

import { ConnectedPosition } from '@angular/cdk/overlay/position/flexible-connected-position-strategy';

Imports which is pointing only to types (*.d.ts) doesn't work. Snowpack shows error:

[snowpack] Package "@angular/cdk/overlay/overlay-config" not found. Have you installed it?

Temporary fixed with type only import

As far as i know, typescript should not emit such imports at all into js file. so it's strange that parcel is complaining about it.

Typescript Path aliases

Additional steps need to be done in snowpack configuration to support typescript path feature. This is more general to snowpack typescript support because I also don't see any ready solutions similar to this webpack plugin It's should be quite easy to develop. So until now i have to duplicate all paths in the config file

// snowpack.config.js
  alias: {
    '@shortcut': './long-path',
  },

SCSS Import issues

Sass allows to have imports in various forms such as

@import "./airline-colors";
@import "airline-colors";
@import "airline-colors.scss";
@import "_airline-colors";

All of the imports above is pointing to the same file. Sass module resultion is diffrent from nodejs and and doesn't treat path without ./ as path to node_modules but on snowpack it seems to be the case.

Snowpack complains about all relative imports which don't have './' in the beginning. @import "airline-colors";

I don't who address is this issue @snowpack/plugin-sass or angular plugin.

ngcc compilation

I see that you implemented ngcc compilation inside the plugin. But until it doesnt play smoothly with snowpack caching proposing adding to readme workaround with ngcc in postintall hook. BTW this is quite popular workaround

PS i see you are using pnmp as package manager. ngcc cli doesn't play well with symlinks which pnmp creates in node_modules see issue

SilverMira commented 3 years ago

Hello, thanks for the detailed feedback.

First of all, yes I do agree that production build with this plugin was never an intended thing. There's no reason to drop angular cli over snowpack other than during development.

As for making it a cli builder, it is certainly a better idea, working in that direction will be right. This plugin is mostly a PoC to demonstrate that there's no reason that Angular can't support using ESM modules, though the main concern of using Angular with snowpack in the first place was with single file building and its complex build tools.

It's better for someone who has deeper knowledge regarding Angular build chain to work on something like this, I've never touched the build tools before making this plugin, so most of the code of this plugin is just a mess gluing what measly information I can find regarding Angular's build API.

The angular.json matter, it seems to be completely my oversight.

As for the typescript imports, I suspect it's snowpack's initial scan of dependencies of the project to build npm packages complaining, though I can't be sure about it.

The path alias is something that does with snowpack's module resolution logic, the same goes for the SCSS imports, so I don't think it's the plugin's responsibility to deal with that.

Regarding the last ngcc notes, the behavior I'm intending for the plugin to do is to automatically detect any packages that requires processing even as it is added midway through development (postinstall wouldn't be enough), which again, doesn't seem to work well with snowpack's current cache management API.

Nonetheless, thanks for the detailed feedback, I will be looking in the Angular builder direction.

Also, I stopped using pnpm for the plugin's development because it was indeed making problems, though it was something regarding snowpack not being able to resolve a pnpm real path. Thanks for the heads-up though.