Skip to content
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

Dependencies check-up #1041

Closed
7 tasks done
Feder1co5oave opened this issue Feb 2, 2018 · 20 comments
Closed
7 tasks done

Dependencies check-up #1041

Feder1co5oave opened this issue Feb 2, 2018 · 20 comments

Comments

@Feder1co5oave
Copy link
Contributor

Feder1co5oave commented Feb 2, 2018

Follow-up from #1020 (comment)

I just removed the node_modules directory and run npm update on a fresh working copy.
This is what I get:

$ npm up
npm WARN deprecated gulp-util@3.0.8: gulp-util is deprecated - replace it, following the guidelines at https://medium.com/gulpjs/gulp-util-ca3b1f9f9ac5
npm WARN deprecated graceful-fs@3.0.11: please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js
npm WARN deprecated minimatch@2.0.10: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated minimatch@0.2.14: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated graceful-fs@1.2.3: please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js
marked@0.3.12 /home/federico/marked
├── eslint@4.16.0 
├── eslint-config-standard@11.0.0-beta.0 
├── UNMET PEER DEPENDENCY eslint-plugin-import@>=2.2.0
├── eslint-plugin-node@5.2.1 
├── UNMET PEER DEPENDENCY eslint-plugin-promise@>=3.5.0
├── eslint-plugin-standard@3.0.1 
├── front-matter@2.3.0 
├── glob-to-regexp@0.3.0 
├── gulp@3.9.1 
├── gulp-concat@2.6.1 
├── gulp-uglify@1.5.4 
├── markdown@0.5.0 
├── markdown-it@8.4.0 
└── showdown@1.8.6 

npm WARN eslint-config-standard@11.0.0-beta.0 requires a peer of eslint-plugin-import@>=2.2.0 but none was installed.
npm WARN eslint-config-standard@11.0.0-beta.0 requires a peer of eslint-plugin-promise@>=3.5.0 but none was installed.

I think this needs some checking.
First of all, I think peerdependencies need to be explicitly installed alongside their dependants, so I'm gonna add eslint-plugin-import and eslint-plugin-promise to #1020`.
This was taken care of by @UziTech in Feder1co5oave#1. Thanks!

Here's the updated section in package.json:

  "devDependencies": {
    "eslint": "^4.15.0",
    "eslint-config-standard": "^11.0.0-beta.0",
    "eslint-plugin-import": "^2.8.0",
    "eslint-plugin-node": "^5.2.1",
    "eslint-plugin-promise": "^3.6.0",
    "eslint-plugin-standard": "^3.0.1",
    "front-matter": "^2.3.0",
    "glob-to-regexp": "0.3.0",
    "gulp": "^3.8.11",
    "gulp-concat": "^2.5.2",
    "gulp-uglify": "^1.1.0",
    "markdown": "*",
    "markdown-it": "*",
    "showdown": "*"
  },

Other concerns I might need some help with:

  • eslint-config-standard is a beta version, should we need to worry about this?
  • gulp-util is a deprecated package. gulp is its dependant. It seems that gulp does not depend on gulp-util since version 4. which is still alpha. What to do? gulp-uglify depends on it too. Update this too? I already updated Gulpfile.js to work with v4, so that would be taken care of.
  • graceful-fs needs to be updated. This can be done updating gulp to 4.0.0
  • minimatch needs to be updated. Same as above, gulp 4.0.0 does this.
  • gulp is used only as a build system to minify the source into marked.min.js, via the plugin gulp-uglify. Can't we use uglify-js directly? gulp comes with a lot of dependencies
  • markdown.js, markdown-it and showdown are listed as devDependencies because we use them to perform comparative benchmarking. Should we bind at least the major version to make sure API don't change over time and break our benching script? In bench against commonmark instead of showdown #1019 I specified version 0.x, for example. Fixed in bench against commonmark instead of showdown #1019
  • remove gulp in favor of using uglify-js CLI. Fixed in Use uglifyjs CLI instead of Gulp to minify #1046

Gulp was brought in in #553 with the hope that it would become more useful over time. In my opinion we should just depend on uglify-js and use it as a CLI in the build npm script.

@Feder1co5oave
Copy link
Contributor Author

Read #1024 too

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

My 2¢:

  • I think using eslint-config-standard is fine in beta. Being that it is just an eslint config I can't imagine there are too many ways it can get bugs. As long as we are willing to stick with this version's rules. (Which I am fine with)
  • I am all for getting rid of gulp. it seems overkill for just uglifying, and one (or in this case multiple) less dependency.
  • I agree with making benchmarking dependencies pinned to a major version.

@joshbruce
Copy link
Member

Throwing copper from me:

  1. I'm okay with using a beta - with the caveat or desire that I hope we can do our best to support perpetuating the community principle mentioned in DEPENDENCIES.md #1024
  2. Gulp is a wonderful tool, imo, but agree - probably overkill for us. I tend to put compilers into three layers: minify (removes whitespace), uglify (shorten property names), and mangle (shorten custom function names). As long as we have the first and second covered using the simplest solution possible - think we're good. (Also, given our low-level concept - we shouldn't have enough code to warrant all of Gulp.)
  3. Agreed on binding to a major for benchmarking.
  4. Agree slightly on explicitly listing the dependencies of dependencies - keeps stuff from becoming hidden ("I only have two dependencies! ...which have 500 dependencies each kinda thing."

@Feder1co5oave
Copy link
Contributor Author

Also, seeing that eslint requires node 4 (active since october 2015) should we require node 4 as the minimum engine?

@Feder1co5oave
Copy link
Contributor Author

@joshbruce the bit about expliciting dependencies was not a proposal, it just needs to be done for npm to install everything correctly.
I like how think alike, us three.

@Feder1co5oave
Copy link
Contributor Author

If we get rid of gulp and use uglify-js instead, we would have these deps:

  1. eslint & co. for linting. These are very well used and maintained.
  2. front-matter and glob-to-regexp help us during testing. Maybe we should do a health check on these, but they're not critical to marked.
  3. uglify-js for minifying on every release, this is very well used and maintained. Have to choose between version 2 and 3.
  4. markdown.js, markdown-it, showdown and/or commonmark.js (bench against commonmark instead of showdown #1019) for benchmarking.

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

The test runner already has some ES6 code that won't work in node <4. but that is just for running tests not using marked.

I think we should require node 4 for the node dependents (like my travis update pr lists) but it would be nice to keep es5 for anyone using marked.min.js

It would be really nice to be able to know if anyone is actually using marked.min.js and maybe drop it if no one is.

It would be nice to use new language features so we should probably use uglify-es instead of uglify-js

@joshbruce
Copy link
Member

@Feder1co5oave: I like the "hive-mind" thing we have going on as well - it's a good team dynamic. And we also have constructive, healthy debates about the work - not each other. Really appreciating this project for those reasons.

My input on updating Node minimum version: I think we had discussed updating the Node dependency elsewhere - I'm not tied to any specific version. (Wish we had a way to ask people who use Marked what version they are on to ensure better backward compatibility.) If we update the minimum version, I will just make it part of the release notes.

Would it be worth discussing a transpiling option?

We can write using es6 - transpile and minify to es5 and es6: marked-es5.min.js, marked-es6.min.js, marked.js kinda thing??

How do we solve this "survey/poll" situation?? Would really like to be able to get feedback from users; otherwise, we're working on best guess assumptions. (My best guess is that most people would use the min if they are doing client-side work...but, as mentioned by @UziTech and @Feder1co5oave elsewhere, we have no way of knowing right now how many people are using marked outside of the web, versus server-side, versus client-side.)

@Feder1co5oave
Copy link
Contributor Author

@UziTech woah slow down a bit.
I think marked.min.js is used by bower/yarn and is available on jsdelivr and such CDNs. I thinks it is a nice option to have and it is easy to generate before publishing to npm. We could even automate this.

I haven't noticed the test runner uses es6. Can you point me to some lines of code that do?

I honestly don't like the transpile es6-to-es5 option. I like keeping it simple, and it would bring up many more dependencies. What I love about marked is that is a small, dependency-free, monolithic and simple library.

@joshbruce from what I can see from the jsdelivr (cdn) page is that marked had 167k "hits" last month, marked.min.js is the main file, and version 0.3.5 is widely the most used. I'd like to find out about this, since that is really outdated.
That's not counting the 3.6M "hits" last month from npm, which uses lib/marked.js as the main file.

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

We can write using es6 - transpile and minify to es5 and es6: marked-es5.min.js, marked-es6.min.js, marked.js kinda thing??

uglify-js can only take es5 code as input. while uglify-es allows inputting es6+ code and outputs es5+ code.

we should be able to just have one marked.min.js output as es5 since all browsers are backwards compatible.

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

@Feder1co5oave the test runner uses an octal literal 0o755

@Feder1co5oave
Copy link
Contributor Author

I've updated the task list.

@Feder1co5oave
Copy link
Contributor Author

Feder1co5oave commented Feb 2, 2018

@UziTech is it safe to publish es6 code to npm? Should we require a certain node version in that case?

edit: ok so no babel is needed, uglify-es takes care of everything. Neat.

I'd like to reiterate that I'm against upgrading to es6 😊

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

node 4 supports some es6. Maybe you are right if we are going to try to support nodev4, marked.js will need to be es5.

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

Really though we already have to create the .min file there wouldn't even need to be another step to transpile marked.js to es5. The npm run build step could take care of creating marked.min.js for browsers and creating marked.js for node in es5.

ES6 classes would help clean up and organize a lot of the code 😄

@joshbruce
Copy link
Member

@Feder1co5oave: Upgrading the core to es6 is a much wider conversation, was just a thought as we continue moving into the future. :) It's possible that by the time we get there, es6 will "be the new black" and the conversation(s) will have been had for us. ;)

Having said that, maybe there's something in using uglify-es over uglify-js to be more "future proof"??

@Feder1co5oave
Copy link
Contributor Author

Ok, I'd go with getting rid of gulp and using uglify-js3 for now. It is a pretty simple workflow and if/when we want to switch to es6 it's very comforting to know that we can do that by (almost only) swapping uglify-js with uglify-es!

uglify-es is API/CLI compatible with uglify-js@3

Could someone help in setting up the prepublish hook in npm to automate the minifying thing? Is it worth it?

@Feder1co5oave
Copy link
Contributor Author

I don't think uglify-es can transpile es6 to es5. Looking at the docs (look for the 'ecma' option):

ecma (default 5) -- set output printing mode. Set ecma to 6 or greater to emit shorthand object properties - i.e.: {a} instead of {a: a}. The ecma option will only change the output in direct control of the beautifier. Non-compatible features in the abstract syntax tree will still be output as is. For example: an ecma setting of 5 will not convert ES6+ code to ES5.

@UziTech
Copy link
Member

UziTech commented Feb 8, 2018

We could use babel with babel-plugin-uglify?

@joshbruce
Copy link
Member

#956

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

No branches or pull requests

3 participants