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

Restructure Codebase #215

Closed
wants to merge 37 commits into from
Closed

Restructure Codebase #215

wants to merge 37 commits into from

Conversation

RebeccaStevens
Copy link

@RebeccaStevens RebeccaStevens commented Nov 12, 2020

  • Refactor code into multiple files
  • Switch to using TypeScript
  • Update dev dependencies

fixes #180
fixes #179
fixes #161
fixes #147

closes #213
closes #211
closes #200
closes #198
closes #188
closes #181
closes #173
closes #148


This change is Reviewable

Copy link
Owner

@TehShrike TehShrike left a comment

Choose a reason for hiding this comment

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

Some quick ancillary notes, I'll circle back and read the actual implementation more carefully later.

One other picky note: I really dislike javadoc/tsdoc/jsdoc function comments, if any of the words in them actually add something, I would rather just move those words into the function names. e.g. if "A function that determins if a type is mergable" is more meaningful, then we should rename IsMergeable to IsTypeMergeable or something.

insert_final_newline = true

[*.md]
indent_style = space
Copy link
Owner

Choose a reason for hiding this comment

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

I do tabs/trim-trailing-whitespace in Markdown too

Copy link
Author

Choose a reason for hiding this comment

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

My reason for switching to spaces in markdown files was just to ensure they render the same regardless of what render is being used - GitHub/npm/other.
But I don't have a strong opinion on this either way. I put this change its own commit so it can easily be dropped.

The reason for not trimming trailing whitespace at the end of markdown files is because two spaces at the end of a line is part of the markdown spec (meaning add a line break here). Official markdown doesn't treat a single line break any different to a single space when it comes to rendering. However, GitHub-flavored markdown, does treat single line break as an actual line break.
If you want to continue trimming trailing whitespace I'm ok with that but I'd recommend not doing so.

Copy link
Owner

Choose a reason for hiding this comment

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

Whitespace at the end of the line can be meaningful in Markdown, but I never use that line-break functionality in documentation

Copy link
Owner

Choose a reason for hiding this comment

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

But the indent_style should definitely be tab 😅

trim_trailing_whitespace = false

[{package.json,package-lock.json}]
indent_style = space
Copy link
Owner

Choose a reason for hiding this comment

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

cursed npm-enforced defaults

.npmignore Outdated
@@ -1,3 +0,0 @@
test/
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know if there's any value in pushing the extra bytes of test files to everyone via npm

Copy link
Author

Choose a reason for hiding this comment

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

I removed the .npmignore file in favor of using the "files" field in the package.json file.

The "files" field works as a whitelist as apposed to a blacklist like the .npmignore file.

I've got the files set to the following (2 folders and 3 files (plus the package.json file itself)):

"files": [
  "dist",
  "types",
  "changelog.md",
  "license.txt",
  "readme.md"
],

You can run npm pack to create the package locally so you can see what's included in it.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah cool, yeah, I like files for that

@RebeccaStevens RebeccaStevens marked this pull request as ready for review November 17, 2020 03:52
"parserOptions": {
"ecmaVersion": 10,
"ecmaFeatures": {
"impliedStrict": true
Copy link
Owner

Choose a reason for hiding this comment

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

Some spaces in indentation in this file it looks like

"rules": {
"no-extra-boolean-cast": "error",
"array-bracket-spacing": [
"error",
Copy link
Owner

Choose a reason for hiding this comment

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

For linting, I use warn for everything and only use error for issues that would actually cause parse-time or run-time errors.

}
],

"@typescript-eslint/prefer-for-of": "warn",
Copy link
Owner

Choose a reason for hiding this comment

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

I detest for-of loop, personally.

I don't want to get bogged down in a full audit of this eslintrc, maybe it could be pulled out to its own PR?

@@ -0,0 +1,49 @@
name: CI
Copy link
Owner

Choose a reason for hiding this comment

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

why move away from Travis in the same PR as all these other changes?

insert_final_newline = true

[*.md]
indent_style = space
Copy link
Owner

Choose a reason for hiding this comment

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

But the indent_style should definitely be tab 😅

@@ -1,5 +1,9 @@
# [5.0.0](https://github.com/TehShrike/deepmerge/releases/tag/v5.0.0)

- Breaking: Endpoint are now exported in esm style [#215](https://github.com/TehShrike/deepmerge/pull/215)
Copy link
Owner

Choose a reason for hiding this comment

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

It's a bit tough for folks to go check out the PR to see exactly what the change was and run into
image

<!--js
const merge = require('./')
const { default: deepmerge, deepmergeAll } = require('.')
Copy link
Owner

Choose a reason for hiding this comment

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

node has always been a first-class consumer as far as deepmerge is concerned – I'm not okay with moving away from

const deepmerge = require('deepmerge')

for node.js/CommonJS

@RebeccaStevens RebeccaStevens marked this pull request as draft December 8, 2020 02:50
@RebeccaStevens RebeccaStevens deleted the branch TehShrike:v5 January 20, 2021 01:35
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