msurdi / frontend-dependencies

Copies node packages to a directory where your frontend tools will be able to find them
24 stars 5 forks source link

updated package.json syntax, install packages via npm automatic #3

Closed FelixFurtmayr closed 6 years ago

FelixFurtmayr commented 6 years ago

Thank you for the good feedback. Please have a look again.

I have one more question:

   if (exact) {

      // please explain this passage
      //////////////////////////////////////
       var sourceIsFile = false;
       try {
           sourceIsFile = fs.lstatSync(sourceFilesPath).isFile()
       } catch (e) { /* don't care */ }

      // if source is a file, create the target parent directory
      if (sourceIsFile) targetPath = path.dirname(targetPath)
      // else: source is a directory, create the full target path
      //////////////////////////////////////

   } else {
       targetPath = path.join(targetPath, pkgName);
   }
   shell.mkdir("-p", targetPath);
   shell.cp("-r", sourceFilesPath, targetPath);

What is the code in the "if (exact)" about - what purpose should it serve?

Second question: I would prefer the "exact" option to be default "true". I would call the option "extraDir" to create an extra folder for all files from one package.

Usually I would just copy one single file from a frontend lib. The option to create an extra folder is very good, but not needed too often. This might be different, if one package has several UI components and you want to select several of them and group them in one folder. What do you suggest?

msurdi commented 6 years ago

Exact is a way to avoid creating a target subdirectory with the package name, if the specified sources are files. Personally, right now I feel like we should remove this option completely, and just "take the decision" of always creating a subdirectory for the package name because this way you avoid overwriting files with the same names for other packages. Suppose you have a library which is bundled as "lib.js" (yes I know, terrible...but could happen) and another which uses the same name... or both bundle something like normalize, etc.. then they will be overwritten.

Seeing that we have just the opposite opinion on this, then there might be use cases for both situations, so maybe having a setting like you described makes sense. I would just call it like "namespace: true/false" or something along those lines rather than extraDir, but that's just personal preference.

FelixFurtmayr commented 6 years ago

"take the decision" I thought about this, too.

For me the question is right now: If we still provide the "exact" option (had no good name for it), what would be the default?

A lib is called "lib.js" it is not named very well. In our projects we could live with that, because we mostly have under 10 frontend libs and have no name conflicts. It's ok to put them in one single folder. Example from one of our apps:

    "angular": "1.5.6",
    "angular-ui-router": "0.3.2",
    "d3": "3.5.10",
    "file-saverjs": "1.3.6",
    "font-awesome": "4.7.0",
    "rf-frontend-common": "0.0.32",

If the average node project uses 20 packages or more, there is a big benefit in a having the extra folder to ensure clean namespace. If the average is 4, than there is no big need for subfolders. If the variance of this number is big ( 50% use 4 libs, 50% use 40 libs), there is a need for the "exact" option.

Could we guess the average number of frontend libs in a usual project? Could we guess the variance in this number?

A bit easier: Can you tell how many frontend libs you use in your projects on average?

This might be the right questions, but this seems to be a detail which can be fixed in the future. Maybe just throw a coin ;-)

msurdi commented 6 years ago

Yeah, completely agree. I'd say just leave the default to put all files together, which I think is what will work the best most of the cases, and if you agree, let's rename the mysterious "exact" option to "namespaced: true/false) so that we can decide if the files for a given package should be put within a directory with the package name. Or do you have a better idea?

FelixFurtmayr commented 6 years ago

I called it now namespaced. After trying to code your suggestion if have a better idea. My suggestion (if you just want to copy single files from a view packages) might have been good in such a case:

      "jquery": {
          "version": "3.1.0",
          "src": "dist/jquery.min.js"
      },
      "normalize.css": {
          "version": "4.2.0",
          "src": "normalize.css"
      }

Now imagine this situation:

      "jquery": {
          "version": "3.1.0"
      },
      "normalize.css": {
          "version": "4.2.0"
      }

Even shorter, but do you really want to copy all files from the packages into one folder? Didn't see this one coming ;-) In the case, that no src option is defined (=> copy whole package) and the namespaced option is not set explicit to false, it will be true, to prevent this problem.

I like this solution now, because the syntax in the frontendDependencies can do all the options you like, but by default is short and will do what makes most sense.

Do you agree?

FelixFurtmayr commented 6 years ago

Another small improvement: After I forgot to uncomment the lines in the test I commented (to prevent the deletion and see the output files), I figured out, that there is no need to delete the files (because then you see what went wrong). You want a cleanup before to avoid old files, so I changed that.

msurdi commented 6 years ago

Awesome! Looks a lot cleaner. As long as we explain the default value change in the README this will make getting started with this a lot easier!

msurdi commented 6 years ago

btw, let me know when you consider everything ready and I will merge and publish the package. Great work! seriously!

FelixFurtmayr commented 6 years ago

Thanks for your commendation :-) I consider it ready - please publish it. I really enjoyed working with you and your good feedback. Was for me the first time, that i used the code review process on gitHub. I really like this.

msurdi commented 6 years ago

Merged and published! One last thing: I'd like you to share the credits of this and your name to be shown in the npmjs.org package's collaborators section in the right sidebar, as I understand that works by adding your details in the package.json "collaborators" key (see here) Would you like to try it?