Skip to content
This repository has been archived by the owner on Dec 17, 2018. It is now read-only.

Dont use package scope everywhere #59

Closed
nvdnkpr opened this issue Jul 21, 2017 · 45 comments
Closed

Dont use package scope everywhere #59

nvdnkpr opened this issue Jul 21, 2017 · 45 comments

Comments

@nvdnkpr
Copy link

nvdnkpr commented Jul 21, 2017

When initializing with npm one can have an (optional) scope along side the command.
e.g. running npm init -f --scope myScope in a directory MyPackageName leads then to a package name @myScope/MyPackageName which is also used for the main module's class name when running the following ngl i

Unfortunately this includes the leading @ from the Scope and the seperator / which Typescript both doesn't allow for class names.
But the solution is easy: Drop the / and the @ and capitalize (I mean the lodash function) the rest which then would lead to a class name MyScopeMyPackageName

@nvdnkpr
Copy link
Author

nvdnkpr commented Jul 21, 2017

I can fix it and send you a pull request, but expect it this evening, since I'm currently in the middle of sth.

@gonzofish
Copy link
Owner

Ah, yes, I wasn't originally supporting scoped packages, but this is the 3rd or 4th person to mention it, so looks like it's worth investing time in. Also #58 looks related.

@gonzofish
Copy link
Owner

gonzofish commented Jul 21, 2017

@nvdnkpr I'm going to take this one because I think there are a few more things to consider than just how the package name is utilized for building the scaffolded module.

Just an FYI, we'll still be using the right side of the / to generate the module name. This means @myScope/cool-package will still scaffold out an cool-package.module.ts.

The build for scoped packages will be slightly different. And it would provide the following to consumers of the package:

|_node_modules/
   |_@myScope/
      |_cool-package/
         |_@myScope
            |_cool-package.js
            |_cool-package.js.map
            |_cool-package.es5.js
         |_bundles/
            |_cool-package.umd.js
            |_cool-package.umd.min.js
         |_*.d.ts
         |_cool-package.d.ts
         |_cool-package.metadata.json
         |_package.json
         |_README.md

The entry points & typings in package.json for a scoped package would be:

{
  "es2015": "./bundles/cool-package.js",
  "main": "./bundles/cool-package.umd.js",
  "module": "./@myScope/cool-package.es5.js",
  "typings": "./@myScope/cool-package.d.ts"
}

Where a unscoped package (e.g. in the package.json, name is just cool-package), the output bundle would be:

|_node_modules/
   |_cool-package/
      |_cool-package.js
      |_cool-package.js.map
      |_cool-package.es5.js
      |_bundles/
         |_cool-package.umd.js
         |_cool-package.umd.min.js
      |_*.d.ts
      |_cool-package.d.ts
      |_cool-package.metadata.json
      |_package.json
      |_README.md

The entry points & typings in package.json for an unscoped package would be:

{
  "es2015": "./bundles/cool-package.js",
  "main": "./bundles/cool-package.umd.js",
  "module": "./cool-package.es5.js",
  "typings": "./cool-package.d.ts"
}

This seems to be inline with how the APF is spec'd.

@gonzofish
Copy link
Owner

This is good, too, because it takes the project in a direction to be able to support monorepos!

@gonzofish gonzofish self-assigned this Jul 21, 2017
@gonzofish gonzofish added this to the Version 1 milestone Jul 21, 2017
gonzofish added a commit that referenced this issue Jul 21, 2017
gonzofish added a commit that referenced this issue Jul 21, 2017
No migration strategy yet...now libraries can have a scoped package name (a la @myscope/my-package) and the library will build in line with the APF
@gonzofish
Copy link
Owner

@nvdnkpr & @tommueller, would either (or both) of you be interested in testing the feature before I publish it? It's a significant change, so I want to make sure it works.

@tommueller
Copy link
Contributor

I will help wherever I can! I cannot really follow the code here though, so I don't know how much I can assist you, but I can test if my project works with the fixed version.

@nvdnkpr
Copy link
Author

nvdnkpr commented Jul 24, 2017

@gonzofish Hello Matt,
if you ask me you shouldn't rush to solve an error (the use of @ and / in a class name are errors) with a enhancement (making angular-librarian a multi-module library seed) . My intention of this issue was to avoid angular-librarian being dropped by other because they use a common feature of npm (=> scopes). And since your work of using scopes was almost fine (except for the class name generation) I just tried to fix the last bit by pointing to something that might be an error.

So please first fix the error and then we can all help you make your library capable of multi-modules, sth that I the other libraries struggle with.

@gonzofish
Copy link
Owner

gonzofish commented Jul 24, 2017

@nvdnkpr my fix is purely aimed at fixing the error you described! I was just mentioning that it is a step towards multi-modules & monorepo support. Also thank you for the offer to help get angular-librarian there 😉

If either of you are interested in helping, here's how you can test!

Install the Branch Version

  1. Remove angular-librarian from your project but don't save it:
npm rm angular-librarian
  1. In your scoped library's pacakge.json, point angular-librarian to the scoped branch:
{
  "devDependencies": {
    "angular-librarian": "https://github.com/gonzofish/angular-librarian.git#scoped",
  }
}
  1. Run npm install to install the scoped branch version of angular-librarian

Verify the Branch is Installed

Your project should now be set up to use the scoped branch version of angular-librarian. To verify that it is:

  1. Open node_modules/angular-librarian/commands/utilities.js
  2. Go to line 25
  3. Verify that the code is:
const checkIsDashFormat = (value) =>
    !!value && typeof value === 'string' &&
    value.length > 0 &&
    value.match(/^[a-z][a-z0-9]*(\-[a-z0-9]+)*$/i);
exports.checkIsDashFormat = checkIsDashFormat;

exports.checkIsScopedName = (name) =>
    // @
    // followed by 1+ non-/
    // followed by /
    // folloer by 1+ non-/
    /^@[^/]+[/][^/]+$/.test(name);

If so, you have the branch version! If not, let me know and I'll try to help.

Use the Library

Run node ./node_modules/angular-librarian i to reinitialize the library.

Now try the same commands you're used to using with angular-librarian. However, you may not be able to leverage use of the ngl command.

To test that a build works without publishing run:

npm run build

If it succeeds, that's a good indicator that the code is working. Check the files output in dist to make sure they seem correct (if you don't know what you're looking for, let me know and I'll help).

@tommueller
Copy link
Contributor

Ok I just did that with a copy of my project and here are my results ;) :

  • I also had to run ngl init again to make it work (missing in your description)
  • the ngl commands still work
  • the build works fine
    • the output looks exactly like the compiled version of the project with the older version, BUT the package-name.js etc. files are now contained within a @scope-folder in my dist-folder. Before they where directly in dist/

Thanks a lot!

@gonzofish
Copy link
Owner

@tommueller Ah, great to hear! Sorry for omitting such a critical step in getting it usable. The output is exactly what we're looking for, too. That lines up with how the Angular Package Format is described. Were you able to use it in a CLI project by any chance?

@nvdnkpr were you going to take a stab at it? If so, let me know if it worked. If not, let me know, too, so I can close this issue. Thanks!

@tommueller
Copy link
Contributor

I will try to use it in a CLI project tomorrow and let you know!

@gonzofish
Copy link
Owner

Thanks, @tommueller, it's worked for me, hopefully it'll work for you too!

@tommueller
Copy link
Contributor

tommueller commented Jul 26, 2017

having some troubles here, but not sure whether this is a problem caused by angular-librarian or by my CI ... don't get why though ... packages are loaded using yarn, so I should definitely have the same version in the CI then locally.
edit: okay forget this, my mistake, did not have the right version in the CI ...

A second issue is, that the npm run g build fails, but the CI does not break runs through and reports to be successful. Is there maybe a process.exit(1); missing somewhere?
Because of this error I cannot test the package in CLI right now, because my published npm-package does not have any content ... I will try to find out why colorize does not work ...

@tommueller
Copy link
Contributor

tommueller commented Jul 26, 2017

Okay I have some more problems. Most of them are probably not specific to this issue, but to the upgrade to beta.9 already, since I couldn't build the package after the upgrade because of the scope thing, I now cannot really tell apart what comes from what.

The package npm generates and wants to publish seems to be broken. Here are the things that are not working for me:

  • for some reason with the upgrade to beta.9 my .npmignore got update and now includes the dist-folder, thus my npm-package does not include any code?! I changed that but now ...
  • ... there is no more index.js file in the dist folder anymore, this is what my compiled dist folder looks llike now:
    unbenannt

where ever I try to import this now, I get

[ts] Cannot find module '@scope/ng-core'.

So now everything is broken completely :)

@gonzofish
Copy link
Owner

Ok let me look into this. You're being a massive help by testing this for me. Your reports are very nicely detailed, too.

@tommueller
Copy link
Contributor

Sure, now worries. You created a great tool that I can use ... ;)

I will be off for holidays by this evening, so I will only be back to investigate further on this by Monday the 7th!

@gonzofish
Copy link
Owner

  • The dist folder isn't going to get packaged because what actually happens is that the tagVersion/publish commands publish package the dist directory--that's why there's now a package.json in dist
  • There hasn't been an index.js in dist for a while--this is just a case of me better learning what should/shouldn't be published. If you look at package.json and look at the typings attribute, that file would be what TS is looking for. So it should point to <your lib>.d.ts which in turn points to all the types for exported members. What may have a problem, however, could be the <your lib>.metadata.json file, so I'm looking into this.

Is colorize still not working for you?

Enjoy your holiday!

@tommueller
Copy link
Contributor

what actually happens is that the tagVersion/publish commands publish package the dist directory--that's why there's now a package.json in dist

that means that if I used npm publish before, now I need to use npm publish dist?

Is colorize still not working for you?

That is working, when I installed the #scoped-branch and ran ngl init afterwards it did override the angular-librarian entry in the package.json, that is why in my CI it installed the wrong version first ...

@gonzofish
Copy link
Owner

that means that if I used npm publish before, now I need to use npm publish dist?

Ah, shoot, yes. Take a look at the posttagVersion script in package.json. That will do what you want it to do. If you use npm run tagVersion, it will do some nice checks to make sure your app works like you think. After that executes, it runs posttagVersion, which just does npm run build && npm publish dist.

Also, good to hear about colorize. I added running node ./node_modules/angular-librarian i after installing the branch. Sorry about that.

@tommueller
Copy link
Contributor

Okay one step further :) The Cannot find module-error is now gone. But now it does not correctly install my peerDependencies anymore ...
In the package.json in the dist-folder the peerDependencies entry is now duplicated!

Also I have one last entry in my package.json which contains an old link to OldPackageName.js, it's called "jsnext:main": "./old-package-name.js", is that critical?

@gonzofish
Copy link
Owner

"jsnext:main" can be removed. Once this was all working, I was going to write up a migration doc which would talk about removing that.

Can I see the generated package.json so I can determine what might've happened? If you don't want to publish your library, you can do the following to test it:

  1. Create a @<scope name>/<library name> directory in your CLI project's node_modules
  2. Build your library using npm run build or ngl build
  3. Copy the contents of the library's dist folder except for node_modules to the CLI projects' node_modules/@<scope name>/<library name> folder you created in step 1.

I just did this and I was able to run ng serve no problem. You, unfortunately cannot use npm link to accomplish this (found via this issue).

@gonzofish
Copy link
Owner

I just tried to ng build --prod --aot and it failed... 😢

@tommueller
Copy link
Contributor

Oh no :)

Btw. I get a lot of these warnings while building the project

No name was provided for external module 'ngx-webstorage' in options.globals – guessing 'ngxWebstorage'
No name was provided for external module '@angular/material' in options.globals – guessing '_angular_material'
No name was provided for external module 'rxjs/Observable' in options.globals – guessing 'rxjs_Observable'
No name was provided for external module 'lodash' in options.globals – guessing '_'

@tommueller
Copy link
Contributor

This is the package.json (only the duplicate peerDependencies part)

  "peerDependencies": {
    "@angular/animations": "^4.0.0",
    "@angular/common": "^4.0.0",
    "@angular/compiler": "^4.0.0",
    "@angular/core": "^4.0.0",
    "@angular/forms": "^4.0.0",
    "@angular/http": "^4.0.0",
    "@angular/material": "^2.0.0-beta.7",
    "@angular/platform-browser": "^4.0.0",
    "@angular/platform-browser-dynamic": "^4.0.0",
    "@angular/router": "^4.0.0",
    "angular2-highlight-js": "^5.0.2",
    "core-js": "^2.4.1",
    "karma": "^1.5.0",
    "karma-chrome-launcher": "^2.0.0",
    "ngx-webstorage": "^1.8.0",
    "rxjs": "^5.0.1",
    "typemoq": "^1.8.0",
    "zone.js": "0.8.12"
  },
  "peerDependencies": {
    "@angular/animations": "^4.0.0",
    "@angular/common": "^4.0.0",
    "@angular/core": "^4.0.0",
    "@angular/forms": "^4.0.0",
    "@angular/http": "^4.0.0",
    "@angular/material": "^2.0.0-beta.7",
    "@angular/platform-browser": "^4.0.0",
    "@angular/platform-browser-dynamic": "^4.0.0",
    "@angular/router": "^4.0.0",
    "core-js": "^2.4.1",
    "ngx-webstorage": "^1.8.0",
    "rxjs": "^5.2.0"
  }

@gonzofish
Copy link
Owner

I bet what happened is this: the package.json is transformed when its copied from the library root to dist. It gets copied, we replace "dependencies" with "peerDependencies". Again, something to be put into migration docs.

What you can do is modify the real package.json in the library's root directory:

  1. Remove the dependencies that angular-librarian added, but make sure all of the versions in peerDependencies match those in dependencies
  2. Change peerDependencies to dependencies
  3. Try to build again, you should have a sane package.json again

@tommueller
Copy link
Contributor

I was just editing the comment:
figured it out, the first peerDependencies entry is the original dependencies entry ... why to you replace dependencies with peerDependencies? shouldnt this be handled by the user?

@gonzofish
Copy link
Owner

Those No name was provided... are also even more documentation. What's happening is we now use Rollup, but Rollup doesn't always know how to handle external libraries (like @angular/material or ngx-webstorage).

If you open tasks/rollup.js, look at lines 18-25. Add the libraries it's talking about in No name was provided... in a similar fashion. Hope that helps.

@gonzofish
Copy link
Owner

why to you replace dependencies with peerDependencies? shouldnt this be handled by the user?

Here's why: when we publish a library, we want to consuming app/library to be in charge of the versions of dependencies so that project-wide, all other libraries/dependencies use the same version. We wouldn't want to specify "@angular/core": "4.1.3" and another project to specify 4.2.2. It'd cause multiple versions of @angular/core to be installed. peerDependencies affords the ability to say "I need this library, but make the consumer install it".

However, when we're developing, we need those dependencies installed, so the root package.json has dependencies instead of peerDependencies. When we copy it over, we just replace the attribute name.

The advice of using peerDependencies comes from @filipesilva's Angular CLI linked library document

@tommueller
Copy link
Contributor

Ok. I was just able (after dirty-fixing the peerDependencies) to import the lib into CLI-project and successfully run ng build, ng test and ng serve.

@tommueller
Copy link
Contributor

tommueller commented Jul 26, 2017

concerning the peerDependencies. As far as I understood, you have to have them both as dependencies and peerDependencies because npm does not install peerDependencies automatically anymore.

And that solves my problem, because then can just remove my peerDepencies entry, because they are duplicated in depencies anyway. So instead of replacing dependecies with peerDependencies you should copy it, I think!

edit: This article was what I read about it back then: https://codingwithspike.wordpress.com/2016/01/21/dealing-with-the-deprecation-of-peerdependencies-in-npm-3/

@gonzofish
Copy link
Owner

So instead of replacing dependecies with peerDependencies you should copy it, I think!

What are you suggesting be copied?

@tommueller
Copy link
Contributor

Copy the dependencies and insert them both as dependencies and peerDependencies. otherwise they will not automatically be installed on the consuming package. See the link above.

@tommueller
Copy link
Contributor

allright. my project is up and running again (right now I still have to manually repair the package.json in the dist-folder, but thats a minor thing) and I will use this rare opportunity to go off into my holidays :)

thanks a lot for the support! and see you in 2 weeks!

@tommueller
Copy link
Contributor

tommueller commented Jul 26, 2017

I just tried to ng build --prod --aot and it failed... 😢

also failed for me 😭
Module not found: Error: Can't resolve 'package-name' in '/builds/ng-apps/ng-app/src/$$_gendir/app/pages'

The package name is without the @scope

@filipesilva
Copy link

Heya, just wanted to chime in: you definitely don't want to add @angular/* dependencies as both dependencies and peerDependencies. Instead you want to add them as dependencies and peerDependencies.

This is important because you really don't want to ever have two copies of them. It's true that if you don't have them in dependencies you aren't guaranteed to have them installed in the consuming package... but that is the point. You don't want to install them. You want to assert they already exist - and that's what peerDependencies are for.

Also, there's a detailed guide in https://github.com/angular/angular-cli/wiki/stories-linked-library about how to link libraries. Hope it helps.

@gonzofish
Copy link
Owner

Hey @filipesilva, thanks for chiming in! The idea is that the published module will have peerDependencies while, during development, they'll be dependencies to more easily facilitate said development. I actually got the idea from that guide you linked!

@filipesilva
Copy link

Heya @gonzofish 👋

I think your approach is good, I was replying to @tommueller's #59 (comment).

@gonzofish
Copy link
Owner

gonzofish commented Jul 26, 2017

Got it, by the way, thanks for that document as well as your filipesilva/angular-quickstart-lib those two things helped me big time in better understanding how to structure Angular library projects

@filipesilva
Copy link

Very happy to hear you found it useful! It took me a while to get my head around it as well, it's not very straightforward but there's definitely a reason for everything.

@nvdnkpr
Copy link
Author

nvdnkpr commented Jul 26, 2017

@gonzofish Sorry for my late reply, I'm currently finishing a project so if I get to it in time I'll rework through the work you and @tommueller have done so far to give you feedback.

@gonzofish
Copy link
Owner

Hey, no problem @nvdnkpr! You're doing me a favor. Any additional feedback is great, but no worries

@gonzofish
Copy link
Owner

@tommueller I've just discovered what the issue was with running ng build --prod --aot

The flatModuleId to in my tsconfig.es5.json & tsconfig.es6.json files was wrong. Instead of being @myscope/scoped-lib it was just scoped-lib. This was not correct. I'll fix it tonight...if you're able to (I know you're on vacation) or @nvdnkpr, you'd like to try it once I update, let me know your findings.

gonzofish added a commit that referenced this issue Jul 26, 2017
@gonzofish
Copy link
Owner

For anyone interested, the following package--produced using Angular Librarian--works with the CLI: https://www.npmjs.com/package/@angular-librarian/scoped-lib

@gonzofish
Copy link
Owner

I'm confident this is working, so I'm going to close. If anyone would be interested in testing for me, please point your angular-librarian install to the master branch:

{
  "devDependencies": {
    "angular-librarian": "https://github.com/gonzofish/angular-librarian.git",
  }
}

And run through the motions. If you reinitialize your project, make sure that you set angular-librarian back to the master branch and redo npm install!

@tommueller
Copy link
Contributor

Sorry I have not been online the last 2 weeks! Thanks for the great work and finishing up on this! I will go on trying the upgrading ASAP.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants