Skip to content

Various maintenance - 0.3.0-dev#28

Merged
kethinov merged 18 commits into
xmldom:masterfrom
kethinov:various-maintenance
Mar 4, 2020
Merged

Various maintenance - 0.3.0-dev#28
kethinov merged 18 commits into
xmldom:masterfrom
kethinov:various-maintenance

Conversation

@kethinov

@kethinov kethinov commented Mar 2, 2020

Copy link
Copy Markdown
Member

Closes #2.
Closes #12.
Closes #20.

Requesting review from @brodybits @timbru31.

Pending approval and merge, we should release this as 0.2.2.

- More package.json refactoring.
- Removed .npmignore in favor of `files` entry in package.json.
- Renamed dom-parser.js to domParser.js.
- Replaced CHANGELOG with more rigorous file.
- Replaced LICENSE with more rigorous file.
- Removed component.json (deprecated package manager https://github.com/componentjs/guide)
- `proof` devDep updated to latest.

Closes xmldom#2.
Closes xmldom#12.
Closes xmldom#20.

Requesting review from @brodybits @timbru31.

Pending approval and merge, we should release this as 0.2.2.
@brody2consult

Copy link
Copy Markdown
Member

Thanks @kethinov for your work on supporting this package, I am sorry that I could not provide very much active support for the recent issues and contributions.

I would personally favor a couple things pretty strongly:

  • new 0.x release (0.3.0)
  • update package engines field to indicate minimum Node.js version consistent with Travis CI update

@kethinov

kethinov commented Mar 2, 2020

Copy link
Copy Markdown
Member Author

Already updated engines field. Altered version to bump minor instead of patch.

@kethinov

kethinov commented Mar 2, 2020

Copy link
Copy Markdown
Member Author

All right, finally got the tests passing. Should be ready for review now.

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A couple more questions:

  • Why remove component.json? Any idea what it was used for, and if there is anything else we should be doing these days?
  • I am a bit ambivalent about committing package-lock.json. My experience is that this kind of lock file locks into outdated dependencies with possible security issues. I use Yarn more often for projects outside of Cordova, actually do have much better experience with yarn.lock (good for quickly moving between various releases with different versions of package dependencies).

Otherwise LGTM. I wish I had more time to understand these updates in depth, oh well.

@kethinov

kethinov commented Mar 2, 2020

Copy link
Copy Markdown
Member Author

As far as I can tell component.json is a manifest file for an obsolete package manager nobody uses anymore. The official docs refer to it as deprecated.

Regarding package-lock, the recommended practice from npm is to commit it. As I understand it, keeping the dependency out of date is seen as a feature in a manner of speaking. This makes it so npm ci can do reproducible builds that always use the same version of every package and every downstream package as well.

Before package-lock files, every time someone ran npm i, different versions of packages or downstream packages might be installed based on whether or not anything in the tree used *, ^, or ~ in its version list, resulting in working builds for some people and broken builds for other people depending on when they ran npm i. Committing the lockfile to the repo makes sure everyone has the same build every time they run npm ci.

@codler

codler commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

Thanks @kethinov for your work on supporting this package, I am sorry that I could not provide very much active support for the recent issues and contributions.

I would personally favor a couple things pretty strongly:

  • new 0.x release (0.3.0)
  • update package engines field to indicate minimum Node.js version consistent with Travis CI update

Can we change to 1.x release?

Comment thread package.json
@timbru31

timbru31 commented Mar 3, 2020

Copy link
Copy Markdown
Member

Thanks for the long overdue cleanup! 💯
I'm fine with both a 0.3.0 or a 1.0.0 release as both indicate breaking changes. What's the your argument for a 1.0.0 @codler?

@codler

codler commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

Thanks for the long overdue cleanup! 💯
I'm fine with both a 0.3.0 or a 1.0.0 release as both indicate breaking changes. What's the your argument for a 1.0.0 @codler?

When you do npm update later on it will upgrade more correctly.

@timbru31

timbru31 commented Mar 3, 2020

Copy link
Copy Markdown
Member

Can you elaborate on that? npm update should not upgrade 0.2.x to 0.3.0, because a minor version bump is considered a breaking change if the major version is zero (0).

See https://semver.org/#spec-item-4 and https://docs.npmjs.com/cli-commands/update.html#caret-dependencies-below-100

@codler

codler commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

for example, 0.2.0 doesnt update to 0.2.1 while 1.2.0 do update to 1.2.1 in case we make a patch.

@timbru31

timbru31 commented Mar 3, 2020

Copy link
Copy Markdown
Member

As far, as I read the npm update documentation, 0.2.1 should be installed in case one specifies ~0.2.0 or ^0.2.0:

Then npm update will install [email protected], because that is the highest-sorting version that satisfies ^0.4.0 (>= 0.4.0 <0.5.0)

@kethinov

kethinov commented Mar 3, 2020

Copy link
Copy Markdown
Member Author

I'm agnostic on the 1.0.0 vs 0.3.0 decision. Let's let @brodybits cast the deciding vote?

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • I think that adding the files entry implies a few more things that should be done, as I said in a comment on the code.
  • I would generally favor splitting the proposed changes into multiple PRs. This case could be an exception, assuming we merge in a single squashed commit.

Comment thread package.json Outdated
Comment thread package.json
@brody2consult

Copy link
Copy Markdown
Member

At this point I would downvote a 1.0.0 release. There are multiple breaking changes proposed in this PR, and I would rather see xmldom become a little more stable before we make the 1.0.0 release.

I would generally recommend that users do the following to get the latest updates:

  • do npm outdated to show which dependencies are outdated
  • do npm add xxx@latest on each non-dev dependency to update (and test before committing and pushing the changes upstream) (--save flag was needed in the past, should not be needed any longer)
  • do npm add --save-dev xxx@latest on each dev dependency to be updated (and test before pushing)

@codler

codler commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

Why wouldn't it be stable when the lib have been around for so long and so many people are using it?

@kethinov

kethinov commented Mar 4, 2020

Copy link
Copy Markdown
Member Author

Regarding the versioning question, I think the tradeoffs work like this:

Argument for 0.3.0: Reflects the fact that we haven't fundamentally altered this codebase. We've cleaned it up a bit, but it's still the beta quality implementation that it was before we hard forked the project. One could say we should reserve 1.0.0 for when we feel the codebase is more mature and stable rather than merely the working but somewhat hacked together quality that it is right now. Basically keeping it 0.x preserves our lack of confidence that the code is hardened and production ready. It slaps a big "use at your own risk" label on the work.

Argument for 1.0.0: This module is used by a very large number of projects. If there were serious flaws in the implementation, we would know by now. Imperfect though it may be, the age of the project and the persistently large user base implies we should just shed our confidence issues and stamp it 1.0.0 as it may have been overdue for being so labeled already.

Personally I lean towards 0.3.0 for the time being, but we should perhaps file an issue to define what it would take to cross the 1.0.0 finish line more rigorously rather than letting gut feelings decide it. Regardless, I don't have a strong opinion on the subject either way, so whatever everyone else thinks I'm cool with.

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with 2 suggestions and 1 more thing for a followup in issue #29

I will raise the issue to discuss 1.0.0 release.

Comment thread CHANGELOG.md Outdated
Comment thread lib/dom-parser.js Outdated
Comment thread lib/dom-parser.js
kethinov and others added 3 commits March 4, 2020 11:22

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @kethinov!

@kethinov kethinov merged commit 004d20c into xmldom:master Mar 4, 2020
@kethinov kethinov deleted the various-maintenance branch March 4, 2020 16:27
@angular-git-user

angular-git-user commented Mar 4, 2020

Copy link
Copy Markdown

Can we move this to 1.0.0? It is breaking our application which has a dependency on
"@azure/service-bus": "1.0.1"
We have a Node version requirement of 8.11.0
and we cannot move to 10.X

@kethinov

kethinov commented Mar 4, 2020

Copy link
Copy Markdown
Member Author

Can you post a stack trace?

The problem is likely with some intermediate dependency that is depending on xmldom using too flexible a wildcard, e.g. ^ or *. In that case, a PR should be submitted to that project to anchor the dependency to ~ or a fixed version.

@brody2consult

Copy link
Copy Markdown
Member

I think this is something that should be fixed in your dependency (@azure/service-bus": "1.0.1").

npm semver with caret (^) should work like tilde (~) for a dependency that is in a 0.x release (exception for 0.0.x).

To quote some resources from the web:

According to https://semver.org/#spec-item-4:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

From https://nodesource.com/blog/semver-tilde-and-caret/#majorzeroandminor10yz0yz0y10:

For versions greater than or equal to 0.1.0, but less than 1.0.0, the caret adopts the same behaviour as a tilde and will allow flexibility in patch versions (only).

For example, ^0.1.3 will permit all versions from 0.1.3 to the next minor, 0.2.0.

I would be willing to consider supporting Node.js 8 for one more minor release in case of sufficient interest. This would need to be discussed in a new issue, not in a closed issue or PR.

@brody2consult brody2consult changed the title Various maintenance Various maintenance - 0.3.0-dev Mar 4, 2020
@brody2consult

Copy link
Copy Markdown
Member

@kethinov you should see that I added a couple milestones:

  • 0.4.0 for followup changes that should be done soon
  • 1.0.0 to track issues and PRs that I think should be resolved before 1.0.0

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.

Changelog? [MAJOR] drop support for some old Node.js versions Travis CI not working

5 participants