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

Upgrade dependencies #60

Closed
wants to merge 3 commits into from
Closed

Upgrade dependencies #60

wants to merge 3 commits into from

Conversation

texastoland
Copy link
Contributor

Fixes broken tests related to testdouble. Also runs standard --fix. Parent commit of other PRs.

@searls
Copy link
Member

searls commented Jul 26, 2018

I appreciate this, but I think if we upgrade deps we should also add a lockfile (e.g. don't ignore yarn.lock), however at this point (for the sake of consistency with most of our other node packages), I'd prefer to lock this down npm and package-lock.json. If you can get a green build with upgraded deps and a package-lock.json, I'd be glad to merge

@rosston
Copy link
Member

rosston commented Jul 26, 2018

Looks like tests are "broken" because the upgraded testdouble package depends on a package using the ... operator, which is unsupported in node < 6. I think getting a green build is probably going to require updating appveyor.yml and .travis.yml. @searls, maybe we switch to node 6, 8, and 10?

@searls
Copy link
Member

searls commented Jul 26, 2018

Thanks @rosston 👍

@rosston
Copy link
Member

rosston commented Jul 26, 2018

@texastoland I tried bumping the node versions on CI in a separate branch, but I see now that failures on newer node versions are why you were updating testdouble along with all the other dependencies. You want to handle updating the node versions in this PR since it seems like that needs to happen in lockstep with the testdouble upgrade?

@texastoland
Copy link
Contributor Author

texastoland commented Jul 26, 2018

@searls I can shrinkwrap no problem. It still makes sense to ignore yarn.lock since Yarn can read package-json.lock too.

@rosston 👍🏽 I'll push shortly. Do we still need to support 6.x? 7.0.0 was released 2 years ago. If I make more PRs beyond the ones today I'd like to clean up the code (e.g. https://node.green/#ES2017-features-async-functions instead of callbacks). Babel wouldn't bother me if it were necessary.

@texastoland
Copy link
Contributor Author

I'd like to clean up the code

@searls assuming you don't mind e.g. same logic with today's best practices (async-await, pure functions, possibly types). Something else I'd really like are Jest snapshot tests. I wonder if you'd mind replacing teenytest.

@rosston
Copy link
Member

rosston commented Jul 26, 2018

@texastoland Yes, I think we should still support node 6. 6 is supported by node until April 2019, and I think our support should more or less mirror node's support. By the same token, 7 and 9 need not be explicitly tested/supported since they have both reached their EOL (due to not being LTS releases).

@texastoland
Copy link
Contributor Author

👍🏽 thank you @rosston I didn't know about EOL dates.

Copy link
Member

@rosston rosston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I'll defer merge responsibilities to @searls though. 🙂

@searls
Copy link
Member

searls commented Jul 27, 2018

Before merge, please revert the changes to delete the trailing \n chars from the handful of scripts in this PR. I still would also prefer not listing yarn.lock in the gitignore. With those assuming we're green I'm happy to merge

@rosston
Copy link
Member

rosston commented Jul 27, 2018

@searls I believe the change only deletes double trailing \n chars, presumably from running standard --fix ("Multiple blank lines not allowed." is a standard rule as of v9).

@texastoland
Copy link
Contributor Author

texastoland commented Jul 27, 2018

👍🏽 confirmed whitespace changes were automated by standard --fix as part of the existing test suite.

👎🏽 why not list yarn.lock? It's popular enough that it's recommended in every React tutorial, generated automatically, but not interoperable with npm. The alternative is any potential contributor should add it to their .git/info/exclude (global would be bad).

same logic with today's best practices (async-await, pure functions, possibly types). Something else I'd really like are Jest snapshot tests. I wonder if you'd mind replacing teenytest.

🛎 are you open to this?

  • Callbacks could be untangled with async/await or vanilla promises (also arrow functions).
  • Repetition logic (e.g. checking fallback directories etc.) could be streamlined with pure functions and reducers.
  • Using modern standards would eliminate Lodash.
  • Types (TypeScript, Flow, Reason, whatever) would have gotten Batch scripts skip directories #59 done in minutes instead of hours because of refactoring.
  • Jest snapshots would make it easier to write tests, better guarantees, and easier stack traces (my single biggest challenge).

@searls
Copy link
Member

searls commented Jul 27, 2018

Thanks @rosston on pointing out that about standard.

@texastoland, I'd only be open to adopting language features in current Node LTS versions, which means no async/await in Node 6, IIRC

@texastoland
Copy link
Contributor Author

texastoland commented Jul 27, 2018

which means no async/await in Node 6, IIRC

Assuming you don't use @babel/node or ts-node (I understand if you prefer not TypeScript) or just compile to dist?

The rest sounds good? If so I request a refactor branch to make smaller PRs. Otherwise the diff will be horrific.

Did you understand my point about yarn.lock? I know about .git/info/exclude but only after years with Git and I've never had to use it. Basically if you don't want a common file versioned it seems apt to ignore it. It can't be globally ignored because other projects need it (or need to ignore it).

@searls
Copy link
Member

searls commented Jul 27, 2018

Correct, this library is not compiled. Any refactors would need to only use libraries that work under Node 6

@searls
Copy link
Member

searls commented Jul 27, 2018

Again, I'd prefer not adding yarn.lock to .gitignore. If I added every possible developer-local file to .gitignore that someone might accidentally add some day (.idea, .code, etc), I'd have to maintain some very long standard .gitignore to copy-paste between all 300 of the repos I own.

@texastoland
Copy link
Contributor Author

texastoland commented Jul 27, 2018

Correct, this library is not compiled

Compilation is part of the refactor question. Today the code is challenging to contribute to in part because it was written without Promises, arrow functions, etc. The point of Babel, et al. is that 2 years from now it won't look like it were written 2 years ago (since Node has been slow to adopt standards). Also no form of static checking would be possible without it.

(.idea, .code, etc), I'd have to maintain some very long standard .gitignore to copy-paste

That exists right? I see your point about not ignoring .vscode. I'll remove that since it can be easily globally ignored (unlike yarn.lock). But Yarn was responsible for more than 6 million downloads last month despite its caching.

🤷🏽‍♂️ I'll do it but that's my opinion as someone actively contributing to JS since before jQuery. I sent a DM in case you want to chat.

@searls
Copy link
Member

searls commented Jul 27, 2018

I'm sorry that the way the library is set up (favoring simplicity over requiring a compilation build step) doesn't suit your tastes, but I'd just ask you to consider deferring to the taste & preferences of the maintainer(s) of small libraries in the future for their sake. Yesterday and today are days off for me (I'm on PTO/vacation) and the 20+ e-mails I've gotten due to your activity on this repo has been a bit of a stressor, and reviewing the issues and points you're raising are work.

I value your desire to help, but appeals to your career's longevity (e.g. "since before jQuery") just seems combative when considering the fact that you're also a guest in someone else's repo.

@texastoland
Copy link
Contributor Author

texastoland commented Jul 27, 2018

Fair assessment. The motivation for asking is because it's not a codebase you're actively working on thus making it more convenient for me (presumably others) if I were to contribute. I'll leave all as is till you can review. Check out https://octobox.io/ as an alternative to GitHub emails. Enjoy your day off.

@jasonkarns
Copy link
Member

jasonkarns commented Jul 27, 2018

Add yarn.lock to your .git/info/exclude if you want to ignore yarn.lock locally.

edit: sorry, didn't catch this comment #60 (comment)

I know about .git/info/exclude

texastoland and others added 3 commits July 28, 2018 15:38
Fixes broken tests related to `testdouble`. Also runs `standard --fix`. Parent commit of other PRs.
And drop EOL'ed versions of node.
@texastoland
Copy link
Contributor Author

👍🏽 roger. Feel free to close the others. I'm going to work on a fork. Thanks for an inspiring project. I think it's more applicable today than when you wrote it.

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

Successfully merging this pull request may close these issues.

4 participants