New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/build a package #1
Conversation
Note strong-cli is removed as a dependency, pending a resolution of slc example not actually being installable due to lb versioning.
`slc example` has been fixed so it now can be `npm install`ed, so use it as a dev dependency.
Current build directory may be temporary, get git cleaned, and the pack output might itself get packed. Avoid this mess by putting the pack file one directory up.
I added the 'onto' git command to this PR. Remaining work is only the committing of build materials. |
steps.push(doNpmPack); | ||
} | ||
|
||
vasync.pipeline({funcs: steps}, callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why vasync
instead of the more common async
? No need to change your code, I am just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed it in the projects by the author of posix-getopt, and the "debuggable internal state" appealled to me. Its API is a bit more clunky, IMO, and I'm not using that particular feature, I confess, because this series is pretty trivial and so far has needed no debugging. However, when using async's queue during strong-mq development, I basically had to wrap every interaction with the queue in a debug layer to track state transitions, so became painfully aware of the difficulty in observing progress of async (or more to the point, lack of progress when things aren't working).
I am worried this may not work well. .gitignore usually contains other local files that should be excluded from the npm package, e.g. I am not sure if there is a solution how to detect which .gitignore entries apply to npm and which don't. Perhaps use special comments to mark which lines to apply? I am proposing to implement one of the following:
|
Other than the comments above, the patch LGTM so far. |
Re: touching of .npmignore. Does npm read "all" the .gitignore configuration sources? Because personal tools such as editor files shouldn't be in a project's .gitignore, I wonder if npm deals with that at all. Anyhow, I believe presence of a .gitignore without a .npmignore is almost always a problem, so I should warn about it. My thinking was: If you always build after doing a 'git clean -xd' (advisable), then you will never have anything in your working tree other than the output of the build, so touching .npmignore is the right thing. I could go the other way, and not touch it, and just warn? Or just warn if there is a .gyp file, or an npm 'build' script? But I don't want to go too far down the path of magically trying to guess the developer's intention. So, summary:
|
I don't think |
Is this at a point where I can try it out on the full stack example? |
@rmg, recall that the packing is done with @ritch Yes. Ping me if you need help, I had trouble, but I didn't know the code as well as you. |
So the thing about having a .gitignore and no .npmignore, is the entire point of this script is to pack build products... and build products are always in .gitignore, so the pack will allways be broken, it won't include build products. If we create a too permissive .npmignore, we package stuff we don't need... but the pack still works. So I added docs and a warning message. I think working but extra garbage is better than not working. |
A missing .npmignore when .gitignore is present, or both bundleDependencies and bundledDependencies in package.json are problematic.
Checking node_modules to avoid optional deps that were not installed is too unpredictable, and always fails if done without first doing an `npm install`. We now do the predictable thing, bundle deps and optional deps. If this isn't sufficient, the user can provide their own bundledDependencies specification in the package.json.
I've manually tested this with sls-sample-app, but would still like to test with the loopback fullstack app. Will work with @ritch on that. |
Agreed. |
Btw, manually tested with full stack loopback example, looks like approach is workable, see strongloop/loopback-example-offline-sync#8 and strongloop/loopback-example-offline-sync#9 |
LGTM. |
/to @rmg @bajtos
See TODO.txt for tracking of WIP (replaces the old gist https://gist.github.com/sam-github/8acbde690ef6b5554e91).
I think this does all the mandatory features, remaining work before release is: