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 all dependencies and remove transpilation step #49

Merged
merged 1 commit into from
Sep 28, 2018
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 23, 2018

This upgrades all dependencies, dropping node 4 support, and bringing in the improvement in create-jest-runner we need for the watch plugin.

It also upgrades the dependency on Eslint, not sure about that one? Feels like it should be a peer dependency anyways, bringing our own is... weird. And the way we load it breaks the assumptions lerna/yarn makes in a monorepo as well (it looks for closest node_modules and expect to find eslint there, instead of walking up the tree to the hoisted one.)

EDIT: Ah, that's #41

EDIT2: Oh, removing the babel step allows us to install from github which is nice 🙂

@SimenB
Copy link
Member Author

SimenB commented Sep 23, 2018

@rogeliog I noticed #48 after I started work on this, so I submitted it anyways 🙂

"build": "babel src --ignore **/*.test.js,integration -d build",
"prepublish": "yarn build",
"format": "prettier --single-quote --trailing-comma all --write \"!(build)/**/*.js\""
"lint": "eslint . --config ./.eslintrc.json --no-eslintrc",
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I’m a big -1 on removing the babel step; we should be using the env preset forever and just changing the targets. Installing from github is antipattern anyways.

@ljharb
Copy link
Collaborator

ljharb commented Sep 23, 2018

(Absolutely both jest and eslint should be peer dependencies, with explicit ranges)

@SimenB
Copy link
Member Author

SimenB commented Sep 23, 2018

The only thing we do syntax wise which doesn't work on node 6 is trailing commas. What benefit does it provide? I suppose we could replace our Object.assign with spread and use async-await? If that's something we want to do (or at least have the freedom to do) I'm not opposed to adding it back in. I did it mostly since it simplified the repo a bit, I don't really care either way.

Installing from github is antipattern anyways.

Fair enough, in this case I wanted to test out the watch plugin branch. Solution was just a yarn/npm link away, so no biggie. (funnily it would have worked if main pointed to src as node 8 runs the code base without issues)

(Absolutely both jest and eslint should be peer dependencies, with explicit ranges)

Cool, we can do that before a release then, bunch up the breaking changes

@ljharb
Copy link
Collaborator

ljharb commented Sep 23, 2018

That at the moment the only syntax a project uses is supported isn’t knowledge that humans should have to think about; that’s what the env preset covers. We should be able to write modern JS at any point and not be tied to v8’s feature release schedule (or node’s v8 update schedule).

In other words, it simplifies the repo’s scripts but ends up making the cognitive overhead for maintainers more complex.

@SimenB
Copy link
Member Author

SimenB commented Sep 24, 2018

Re-added babel (although using babel 7 instead of 6). Difference in output after looking at the diff is that we now use arrow functions and const over var

@@ -3,12 +3,14 @@
## 0.6.0

### Fixes
* cli options rules should be passed as an object instead of array of strings ([#39](https://github.com/jest-community/jest-runner-eslint/pull/39))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just running prettier on markdown files? Can we revert the unrelated changes? Prettier makes markdown uglier imo, except for tables.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is prettier, yeah.

What's uglier about it? It's just swapping * for -.

I'm of the mind to kust let prettier do its thing rather than caring. Just set and forget

Copy link
Collaborator

Choose a reason for hiding this comment

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

This project doesn't currently use prettier; but also yes, the - is less readable in raw form than the *.

imo, a "set and forget" mentality abdicates responsibility for ensuring things are readable :-/

Copy link
Member Author

@SimenB SimenB Sep 25, 2018

Choose a reason for hiding this comment

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

We do use prettier; we have a prettier in package.json, a prettierignore and the eslint plugin for it - all on master

Copy link
Collaborator

Choose a reason for hiding this comment

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

"prettier in package.json" is added by this PR.

I do see npm run format; either way, i'd expect people to have editors run eslint autofix, but not to blindly run prettier on all files, and i'd prefer not to run it on markdown files.

Copy link
Member Author

Choose a reason for hiding this comment

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

"prettier": "1.5.3"

That's master?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, i see the dep - but not the config, which is what confused me.

In master, prettier only runs on JS files - i'd prefer to keep it that way.

Copy link
Member Author

@SimenB SimenB Sep 28, 2018

Choose a reason for hiding this comment

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

fair enough 🙂 I'll keep the changes it made though

Copy link
Collaborator

Choose a reason for hiding this comment

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

:-/ the changes it made are what i have a problem with.

@@ -2,5 +2,4 @@ language: node_js
node_js:
- '10'
- '8'
- '7'
Copy link
Member Author

Choose a reason for hiding this comment

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

eslint refuses to install: error [email protected]: The engine "node" is incompatible with this module. Expected version "^6.14.0 || ^8.10.0 || >=9.10.0".

We could ignore engines, but I don't think that's better than dropping a version of Node that has been EOL since summer 2017

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's fine not to run on EOL never-LTS versions of node imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point on differentiating between what was LTS at one point and what was not

@SimenB SimenB merged commit 3fc5ab8 into master Sep 28, 2018
@SimenB SimenB deleted the bumpybumpp branch September 28, 2018 07:09
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.

3 participants