scttcper / ngx-color

🎨 Color Pickers from Sketch, Photoshop, Chrome, Github, Twitter & more
https://ngx-color.vercel.app
MIT License
436 stars 55 forks source link

fix: Angular 13 exports #360

Closed Timebutt closed 3 years ago

Timebutt commented 3 years ago

Hi @scttcper, this PR addresses the issue I mentioned yesterday where version 7.3.0 of ngx-color would not run correctly in modern build environments because of the incorrect exports configuration.

This time around, I made changes to the repository to properly implement the recommended ng-packagr way of doing multi-entry-point packages (see their documentation). I ditched the custom build system, npm run build now leverages the strenghts of ng-packagr to give us the most modern APF package currently available! I even made improvements so that peerDependencies only required for two packages, are also marked as peerDependencies only those two sub packages, not for the entire ngx-color.

I also tested it in a real-world application this time, and it worked just perfectly ;) Sorry for not testing the package before proposing my previous PR, but I incorrectly assumed using ng-packagr would deliver a correct package when using a custom build.

scttcper commented 3 years ago

:tada: This PR is included in version 7.3.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

scttcper commented 3 years ago

Thanks for figuring that out, just tried it with a new 12 project, seems fine

spicemix commented 3 years ago

Thanks for this fix. However, you may have shaved off too many dependencies in practice:

Errors when compiling in 7.3.1:

./node_modules/ngx-color/fesm2015/ngx-color-circle.mjs:7:0-178 - Error: Module not found: Error: Can't resolve 'material-colors' in '[MyProjectDir]/node_modules/ngx-color/fesm2015'

./node_modules/ngx-color/fesm2015/ngx-color-circle.mjs:8:0-44 - Error: Module not found: Error: Can't resolve '@ctrl/tinycolor' in ''[MyProjectDir]/node_modules/ngx-color/fesm2015'

./node_modules/ngx-color/fesm2015/ngx-color.mjs:7:0-44 - Error: Module not found: Error: Can't resolve '@ctrl/tinycolor' in ''[MyProjectDir]/node_modules/ngx-color/fesm2015'

Are we now supposed to install those ourselves? Is that in the new documentation?

spicemix commented 3 years ago

I have confirmed everything works in 7.3.1 in Angular 12 once @ctrl/tinycolor and material-colors are manually added to the package.json. Therefore the documentation needs to be updated to match this new required action on the part of the developer. Thank you!

scttcper commented 3 years ago

I didn't have that issue with angular 12. You might try removing your node modules and reinstalling

spicemix commented 3 years ago

They are now peer dependencies. Presumably this is because not every installation will need them. But because they are peer dependencies they must be specifically installed by the developer in addition to ngx-color. They won't be installed automatically although the package manager may warn you they are missing. In my case it warned about @ctrl/tinycolor but not material-colors. Manually adding both of those got me back up and running.

scttcper commented 3 years ago

I see what you're saying. I'll take a look later

scttcper commented 3 years ago

Looks like the readme was dropped from npm as well

Timebutt commented 3 years ago

@spicemix is correct in stating they are now peerDependencies. I gave it some thought meanwhile, and while in general it is definitely the recommended way to configure dependencies for a package, in this specific case I think it does indeed make more sense to add ngx-color as well as @ctrl/tinycolor as dependencies of their respective packages.

Neither ngx-color or @ctrl/tinycolor are likely to be used by other packages in the dependency tree of an application, and as such I'll propose a PR to reinstate to the way things were before. A case of pragmatism and developer experience over 'the ideal world scenario'.

Also, I didn't know the README.md and LICENSE.md explicitly need to be in the package for them to appear on npm, I figured it pulled them from the linked GitHub repository, I'll fix that as well in the package ;)

Thanks for the feedback, this is a great case of failing forward I guess!