Closed DickSmith closed 6 years ago
Hey Dick, Thanks for the PR.
What is the benefit of removing the slides directory? I initially had that setup so someone working on the project can work on the slides project with quick reloading. instead of making a change, then running the demo script, to see said change on their phone or emulator.
Also is there any way you can break this up into a couple commits? it will make going through it much easier. Thanks again, I appreciate you taking the time to look into this issue. I've been busy with other things and haven't had a chance too.
@TheOriginalJosh
Hey, yeah, the main reason was just to make it easier to pack up and distribute directly from src/
without having to change directories or move stuff around manually, and just make it clearer where the source code was and reducing some of the complexity of the structure, which I think may have lead to my original issue.
The other advantage is that running it in the external demo also makes it more accurate to the behaviour that would occur in having it used as a node_module rather than directly inside the app. I also assume maintaining 2 "demos" is more difficult.
The changes look big, but 95% of it was just deleting the second app structure, and the other 5% was just moving around the structure of the code and some changes to the package.json/tsconfig.json/.npmignore/.gitignore to make compiling/packing the distributable more transparent and predictable. If you look at the diff for the slide code files, the only change is just moving them down onto /src
directly.
tl;dr: To make it more similar to the structure of other plugins, simpler to identify where the plugin code begins/ends, make packing/distribution much easier, and make running/testing changes more similar to regular usage as a node_module
. Essentially, now to build the plugin from scratch and publish would just be cd src && npm i && npm publish
(npm publish will then call the prepack
/postpack
hooks to ngc
and include the LICENSE
and README
in the distributed package).
Thanks again for all your work on this plugin. 😀 Let me know if you still have any concerns/reservations about this PR.
Thanks for the explanation, However I really do not want to lose the slides working directory, unless absolutely necessary, I have had this project setup like this before and it simply felt like it made changes much more tedious and time consuming
The other "demo" app is simply for verification purposes and like you said testing it in a more real world scenario.
One thing I have been looking at has been to move the project into something like ngx-packagr or with the new library generator included in the CLI however I just haven't had time to actually implement it in this project.
I'm sorry however I'm going to close this PR for the time being since it removes the working directory. I may be able to look into this more this weekend, or if you would like to open another PR that doesn't drop the slides working directory that would be great as well.
Thank you for taking the time to look into this.
@TheOriginalJosh
No trouble, I understand completely, I was aware that my PR was a bit too opinionated, but was just the easiest way for me to sort out the packing issues; likewise I just fell back on the app structure and build processes I'm most accustomed to, but thought I would PR it just in case it would be of any use to you.
I think the man changes that resolve my issue were to the tsconfig.json
, package.json
.gitignore
and .npmignore
(which inherits from .gitignore
so has to have rules to include/exclude anything into the package that would differ from the .gitignore
).
Thanks again. 😀
Cleaned up some of the compilation settings, simplified the structure, and removed the extra
app/
stuff. Runningnpm pack
/npm publish
should now produce a complete lib (including README and LICENSE) No code/functionality/breaking changes (still compiled against ng5).