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

Update dependencies #513

Merged
merged 26 commits into from
Jul 31, 2020
Merged

Update dependencies #513

merged 26 commits into from
Jul 31, 2020

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jul 28, 2020

Rollup Plugin Name: *

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

This updates all dependencies for all plugins, using one commit per plugin. At the moment, there are some security issues that prevent PRs from passing CI.

@lukastaegert lukastaegert force-pushed the update-dependencies branch 2 times, most recently from 7f3fa66 to 6f44561 Compare July 28, 2020 09:47
.eslintrc.js Outdated
plugins: ['prettier-plugin-package']
}
]
'prettier/prettier': 'error'
Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved prettier config to a dedicated file that is shared in all places and provides better IDE support. Relevant change is probably that the printWidth in README files is now 100 characters as well. If this is not desired, I can add an override for markdown files to set it to 80 again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah please do revert this. This configuration keeps prettier and ESLint from fighting, and requires only a single editor extension. There's a lot of debate about this, but reverting this change is going to cause more trouble than it's worth.

Copy link
Member Author

Choose a reason for hiding this comment

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

And if we just double the config from the .prettierrc file in the .eslintrc.js file? In any case, I would really love to get rid of the manual prettier --print-width params in favour of central config. As the eslintrc is central as well, that would be fine with me.

@@ -6,8 +6,8 @@ export default {
input: 'src/index.ts',
external: [...Object.keys(pkg.dependencies), 'os'],
output: [
{ file: pkg.main, format: 'cjs' },
{ file: pkg.main, format: 'cjs', exports: 'auto' },
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are to silence a warning in latest Rollup.

{ file: pkg.module, format: 'es' }
],
plugins: [typescript()]
plugins: [typescript({ sourceMap: false })]
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently the TypeScript plugin complains if I do not generate a sourcemap unless this is set to false. I assume this will also improve performance.

"scripts": {
"build": "rollup -c",
"ci:coverage": "nyc pnpm run test && nyc report --reporter=text-lcov > coverage.lcov",
"ci:lint": "pnpm run build && pnpm run lint",
"ci:lint:commits": "commitlint --from=${CIRCLE_BRANCH} --to=${CIRCLE_SHA1}",
"ci:test": "pnpm run test -- --verbose",
"lint": "pnpm run lint:js && pnpm run lint:docs && pnpm run lint:package",
"lint:docs": "prettier --single-quote --write README.md",
Copy link
Member Author

Choose a reason for hiding this comment

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

These options are now handled via the .prettierrc file

@lukastaegert lukastaegert force-pushed the update-dependencies branch 4 times, most recently from f48e3e0 to 5b2a9e7 Compare July 28, 2020 14:24
@lukastaegert lukastaegert marked this pull request as ready for review July 28, 2020 14:24
@lukastaegert
Copy link
Member Author

This is now ready for review

@shellscape
Copy link
Collaborator

@lukastaegert I'll take a look at this tomorrow, was out in the Gulf of Mexico all day today and I'm whooped. Thanks for doing this.

@lukastaegert
Copy link
Member Author

Sure, no problem, I envy you! There are some changes I made that you should think about if it is ok, mainly the prettier change. On the other hand, I do not think there is a need to release new versions of any plugins just because of this.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Definitely want to revert the prettier changes.

Some additional comments that are probably OK but need some explanation or reply.

I really want to double check that none of the build config updates are going to change functionality of the shipped package. We also need to assert that none of the updates to node-resolve are going to cause downstream issues. Hoping our tests are comprehensive enough, but please do give a second look and a 👍 if you're confident.

@@ -1 +1 @@
!.eslintrc.js
.eslintrc.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason this was changed? we'd like ESLint to ensure that the config file matches style as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was getting a "Parsing error: 'parserOptions.project' has been set to @typescript-eslint-parser'. The file does not match your project config: .eslintrc.js.
The file must be included in at least one of the projects provided."
If you have an idea to make it work, I have not.

.eslintrc.js Outdated
plugins: ['prettier-plugin-package']
}
]
'prettier/prettier': 'error'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah please do revert this. This configuration keeps prettier and ESLint from fighting, and requires only a single editor extension. There's a lot of debate about this, but reverting this change is going to cause more trouble than it's worth.

packages/auto-install/rollup.config.js Show resolved Hide resolved
packages/beep/package.json Show resolved Hide resolved
packages/dynamic-import-vars/rollup.config.js Show resolved Hide resolved
import pkg from './package.json';

export default {
input: 'src/index.js',
plugins: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason for removing the babel plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

As we now have a Node 10 baseline, I do not think it makes sense to explicitly transpile down to loader compatibility targets as it both bloats files and makes them less readable. You may have hit an important point here, though: Technically, this would make this a breaking change as we would lift the compatibility baseline to Node 10. I would still love to keep this change but add a "BREAKING CHANGE" note to the commits if you agree. No need to release a new versions right away, though, IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please feel free to update the engine to v10 in package.json on a new commit for the plugin (and any others that were changed with this) as a breaking change. Totally worth a major, and we've been kicking the can down the road on doing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the engines property into the original commits and added them as breaking changes to the commit messages

packages/pluginutils/package.json Show resolved Hide resolved
@lukastaegert
Copy link
Member Author

Definitely want to revert the prettier changes.

There is more to it than just reverting because some changes need to be done. Basically what prettier 2 does is it changes a few defaults that do not fit our current code-style. If we do not want a .prettierrc file, then we need to amend each call to the prettier CLI in every single package.json file with a --trailing-comma none and possibly an option for --arrow-parens as well, making this less and less maintainable.

Also it does not use a chance we have with the mono-repo to enforce consistent styling with shared configuration that is easily updated in a central place.

I added another commit to show-case that this is also very flexible by enforcing the previous print width of 80 for README files only. I can of course merge this into the previous commits again, this is just for demonstration:

{
  "arrowParens": "always",
  "printWidth": 100,
  "singleQuote": true,
  "trailingComma": "none",
  "plugins": ["prettier-plugin-package"],
  "overrides": [
    {
      "files": "README.md",
      "options": {
        "arrowParens": "avoid",
        "printWidth": 80
      }
    }
  ]
}

This will also be picked up by ESLint but also has better tooling support so that you can have e.g. prettier on save instead of having to trigger eslint manually time and again. While I agree this is strictly not necessary, I would urge you to use this chance since I needed to touch everything anyway to reduce maintenance overhead here and improve consistency.

.prettierrc Outdated Show resolved Hide resolved
@shellscape
Copy link
Collaborator

prettier on save instead of having to trigger eslint manually time and again

That's the part that confuses me. Atom, VSCode, VIM, and other editors all have an ESLint plugin that will run automatically. It negates the need for running prettier independently - and it honestly cleans up the conflicts between ESLint and prettier rather well. Prior to merging the two, they were constantly at odds. That's why we went that direction. I recently introduced the same change at work and we've got ~50 devs working the same way daily, with a big improvement in DX. There are different schools of thought on this, but I'd really encourage not trying to change the status quo that we've been operating on in the plugins repo for a good while.

If Prettier@2 is the source of the problem, then is there a compelling reason to update that?

In the mean time, I'll take a look at Prettier@2 and see what's up with the markdown files.

@lukastaegert
Copy link
Member Author

No really, it was just two relevant defaults that are different. I can add the flags to the manual calls in the package.json files.

@shellscape
Copy link
Collaborator

Gotcha. Before you do that, please give me a bit to see if there's another way to go about that. I have an idea I need to test.

@lukastaegert
Copy link
Member Author

Ok, then I will not do that yet. Thanks for giving a little more background about why it was handled that way.

@lukastaegert lukastaegert force-pushed the update-dependencies branch 2 times, most recently from 9feccb6 to efcf371 Compare July 30, 2020 05:19
@lukastaegert
Copy link
Member Author

I did revert the prettiert changes now, in the end it was really only one additional parameter. I hope this can be merged now. Only open point is the eslintignore that is still excluded due to issues with the typescript parser.

@shellscape
Copy link
Collaborator

Sorry for the delay. I'm on the tail end of a 15 hour day today and I'm wiped. You aren't forgotten I promise!

@lukastaegert
Copy link
Member Author

No worries.

@shellscape shellscape merged commit 4c65d01 into master Jul 31, 2020
@shellscape shellscape deleted the update-dependencies branch July 31, 2020 13: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.

2 participants