mtrutledge / generator-dnn

Yeoman generator to scaffold DNN / DotNetNuke Modules and Themes.
MIT License
17 stars 10 forks source link

Gulp Package Uses Wrong Version Number #29

Open SkyeHoefling opened 6 years ago

SkyeHoefling commented 6 years ago

I believe the packageSource and packageInstall build tasks are creating the incorrect file name and makes it confusing if you are not familiar with module versioning. Since we are using npm we technically have 2 manifest files:

The dnn manifest is what really matters as far DNN is concerned. When I tried incrementing the version in the package.json the dnn manifest was not incrementing with the command gulp package command. Which is correct in my opinion, BUT the file name version was incrementing with that. When I looked through the dnn-package.js code I saw that it is pulling the version from the npm package.json and not the dnn manifest.

Here is a small excerpt of the dnn-package.js file to highlight my point

var gulp = require('gulp');
var config = require('../../package.json');

...

gulp.task('packageInstall', ['build'], function() {
  var packageName = config.dnnModule.fullName + '_' + config.version;

...

gulp.task('packageSource', ['build'], function() {
  var packageName = config.dnnModule.fullName + '_' + config.version;
...

I can see good arguments for making a gulp task to increment the version in the manifest file. I personally see this as a bad idea or at least it should be made optional. If you use any type of build environment or additional scripts to track different versions of your module the manifest defines everything. In my build environments I use a script that increments manifest version number to match the build number. If the gulp package overwrote that it would be a headache to get the order of operations just right.

I only investigated MVC but I am assuming SPA is exactly the same

Requested Change/Discussion

I think we should update the dnn-package.js to pull the version from the DNN manifest file instead of the npm manifest file package.json

I would like to discuss this more if there are disagreements on my requested change

mtrutledge commented 6 years ago

I do believe the version of the Dlls is set through the package.json configuration via a gulp task that updates the Assembly Info. So if the manifest is not being updated that is a bug. The initial idea was that gulp would drive the build process. Eventually, I would like the dnn manifest file to be generated for the user so they do not have to manage it. For example, the program would store the user's information in the package.json file and then during build a process would look for any folder with a View file in it and consider that a module and add it to the dnn.manifest file and then package the installer. I also believe that is the way nvQuickTheme is generating the manifest file (Uses config values from the package.json and populates the dnn.manifest on build)

SkyeHoefling commented 6 years ago

I have some problems with the package.json managing the manifest file.

There is going to be a fine line between making things easier and having gulp tasks do it all for us and taking away control from people where the tool might not be as useful.

With all that being said I see a lot of merit in having gulp manage the dnn manifest file. I personally find it cumbersome in how my modules are managed from a build, versioning and manifest file perspective.

This is going to require more thought.

mtrutledge commented 6 years ago

It will be hard to strike a balance. There may have to be some gulp tasks that are run optionally. If someone uses the npm version command then there should be a hook that would update the manifest versions after the version has been patched.

Or maybe we give instructions to disable a build step if the user wants to manually manage their manifest file. That might be a really good option considering this tool was meant to attract some newer developers.

mtrutledge commented 6 years ago

Looking at nvQuickTheme their gulp build tasks also manage the information in the manifest file

david-poindexter commented 6 years ago

That is correct @mtrutledge - we also have a separate JSON file for making project setup a bit easier. That way people do not have to understand all the intricacies of the DNN manifest file.

SkyeHoefling commented 6 years ago

My question is where does this leave the power users? The people that are comfortable modifying the manifest file? They need to find portions of the gulp tasks and update them to not overwrite their changes

mtrutledge commented 6 years ago

After some discussions, we are going to change the packageInstall and packageSource to pull the version from the manifest file. Then we are going to add a manifest task that will update the version of the manifest.

SkyeHoefling commented 6 years ago

This is a great compromise where we can still take advantage of all the gulp scripts but still take into account the power users that want to manage the manifest themselves.

Do you want help with this one? We should just need an xml reader that we include and grab the file version. The only issue that may come up is with old manifest files the version isn't set the same way, but I don't think it is a problem we will run into with this tooling. As a safeguard we could follow this workflow:

  1. Check the dnn manifest file first
  2. Check the node package.json file 2nd

this way we have a fallback in case the manifest file is messed up for whatever reason