langleyfoxall / technologies

Technical overview of Langley Foxall, related technologies, frameworks, style guides and more.
https://technologies.langleyfoxall.co.uk/
Other
8 stars 8 forks source link

Open source requirements #87

Closed AlexCatch closed 2 years ago

AlexCatch commented 5 years ago

As we're creating more and more open source packages, it makes sense to discuss and agree on a set of standards and requirements that have to be met before a package has a v1.0.0 release.

This is important because when you make an open source package under the company Github, you're representing the work we as a company can produce and are proud of.

I think at a bare minimum we should have the following

These are packages that other developers could be using in production systems and expect it to work reliably so I feel as a v1.0.0 release indicates that the project is ready for production, the above should be the minimum.

For versions below v1.0.0 I think we should have the following requirements from the beginning.

cc. @wheatleyjj @DivineOmega @dextermb @ash123456789 @NilesB

dextermb commented 5 years ago

Travis process to ensure changes to the code base doesn't break existing functionality.

While I agree that testing is required, with NPM you're able to test locally before publishing which should be enough.

{
  "scripts": {
    "lint": "standard 'src/*' 'test/*'",
    "build": "del dist/* && babel src -d dist",
    "prepublishOnly": "npm run test",
    "pretest": "npm run lint && npm run build",
    "test": "npm run testOnly && npm run testTypings",
    "testOnly": "jest \\.test.js --coverage",
    "testTypings": "tsc --project ./typings/tests",
    "testCI": "npm run test && cat coverage/lcov.info | coveralls",
    "dev": "concurrently -k 'npm run build -- -w' 'npm run testOnly -- --watch'"
  }
}

Source.

Anything under the Langley Foxall namespace on packagist and NPM should follow styleguide standards.

AlexCatch commented 5 years ago

@dextermb We'd then face the same issue we face with our commercial projects, it's very easy to forget to run your test suite before a push etc, Travis takes out the need to remember to run your test suite and acts as a safety net to ensure your code 100% works.

As other devs could possibly rely on the project outside the company, I'd say it's very important to run it through a CI solution because if one of our open source projects breaks another developer's project because the maintainer forgot to run their tests that's a nightmare.

dextermb commented 5 years ago

@dextermb We'd then face the same issue we face with our commercial projects, it's very easy to forget to run your test suite before a push etc, Travis takes out the need to remember to run your test suite and acts as a safety net to ensure your code 100% works.

As other devs could possibly rely on the project outside the company, I'd say it's very important to run it through a CI solution because if one of our open source projects breaks another developer's project because the maintainer forgot to run their tests that's a nightmare.

Thankfully NPM has scripts that are automatically ran within the lifecycle of most NPM actions. Unless we add more processes to Travis then I'd advise not putting JavaScript packages on there as NPM will cover testing automatically.

AlexCatch commented 5 years ago

@dextermb We could do with fleshing out what would go in Travis but a general rule of thumb for my projects is:

Travis is there to run the above steps to ensure that the project stays consistent etc, relying on the local setup of each dev to ensure that test & linting passes etc isn't more reliable than an independent, consistent third party running everything automatically in an environment that doesn't change.

In next-routes they also use a CI solution in conjunction with NPM hooks I would assume to ensure that any new code doesn't break the projects of those that rely on it.

I think having both would make the project bulletproof to those who rely on it.

Defense In Depth

dextermb commented 5 years ago

@dextermb We could do with fleshing out what would go in Travis but a general rule of thumb for my projects is:

  • Bundle the project through something like npm run prod to ensure the project bundles fine
  • Run the tests
  • Lint the project to ensure it meets our style guide(s).

Travis is there to run the above steps to ensure that the project stays consistent etc, relying on the local setup of each dev to ensure that test & linting passes etc isn't more reliable than an independent, consistent third party running everything automatically in an environment that doesn't change.

In next-routes they also use a CI solution in conjunction with NPM hooks I would assume to ensure that any new code doesn't break the projects of those that rely on it.

I think having both would make the project bulletproof to those who rely on it.

Defense In Depth

I'm sure all that Travis would do is to run the same commands that NPM would run? I don't get the benefit to having a third party run the same tests that are enforced by NPM. If tests fail during publish it will cancel the publish.

AlexCatch commented 5 years ago

It's dependent on the developer's setup which is the issue, easy to have something like different versions of NPM installed with their own different intricacies etc, no two developer machines are the same.

I'm not saying not to use it, it's great, but there is no downside to have Travis run the build as well.

If something goes wrong with NPM, however unlikely due to the setup of the developer machine, Travis is there to catch it. If something is up with Travis, NPM catches the failing tests before Travis.

It would be easier to set travis to run a job on tagging of a new version to run everything independent of the developer's setup and to publish to NPM on the successful completion of the build etc.

I've had times in a project where tests pass on my machine but don't pass on another developers etc, the key is that you have a consistent environment to run your tests in.

dextermb commented 5 years ago

I'm not saying not to use it, it's great, but there is no downside to have Travis run the build as well.

πŸŽ‰ Awesome, for now I think we should implement some tests with prepublishOnly in package.json and look into Travis once we have more seats.

It's dependent on the developer's setup which is the issue

...

I've had times in a project where tests pass on my machine but don't pass on another developers etc.

That's interesting. I'd assume that tests should test codebase functionality that would be affected by the host machine. For example testing helper functions would always return their values separate from whatever host machine is running.

AlexCatch commented 5 years ago

@dextermb Travis is free for open source projects so we don't have to worry about it taking up time for our own commerical builds as it runs on a completely seperate Travis site

On the testing side, it's mostly down to intricacies of Laravel Dusk etc, it wouldn't happen as often with unit / feature testing but could easily come down to the different setup of the test runners etc that's not so much an issue with two developers but can easily get out of hand with the more developers you have.

dextermb commented 5 years ago

@dextermb Travis is free for open source projects so we don't have to worry about it taking up time for our own commerical builds as it runs on a completely seperate Travis site

On the testing side, it's mostly down to intricacies of Laravel Dusk etc, it wouldn't happen as often with unit / feature testing but could easily come down to the different setup of the test runners etc that's not so much an issue with two developers but can easily get out of hand with the more developers you have.

That's all good then. I hope you wouldn't be using Dusk for NPM packages.

AlexCatch commented 5 years ago

Never say never ;)

NilesB commented 5 years ago

Another thing to add is that packages should be fully documented before the proper release.

NilesB commented 5 years ago

Also a note on versioning. 0.x.x should only be used for initial development. If the package is being tested and needs to be released on npm the versioning pattern x.x.x-(alpha | beta) should be used. By default npm will initialise with 1.0.0 which has caught me out so that's worth watching.

dextermb commented 5 years ago

Also a note on versioning. 0.x.x should only be used for initial development. If the package is being tested and needs to be released on npm the versioning pattern x.x.x-(alpha | beta) should be used. By default npm will initialise with 1.0.0 which has caught me out so that's worth watching.

I think we'd use a version with -alpha or -beta when we publish to NPM for internal use, before meaning to go public.

For example: 1.0.0-alpha would be used for internal use. This would be used on the @langleyfoxall/react-dynamic-context-menu as it's not ready to be used by everyone but is required in one of our projects.

AlexCatch commented 5 years ago

in eloquent-csv-importer, I used 0.0.1-alpha and worked up from there.

In initial development, I like to start as small as possible and then follow semver up.

In the future it would make sense to start at 1.0.0-alpha and marked as a pre-release if making releases on Github before v1.0.0.

v1.0.0 should be considered stable and usable in production, anything below v1.0.0 should be considered not production ready etc.

NilesB commented 5 years ago

Using 0.0.1-alpha is fine for packages being built from the ground up with open source in mind. But if the code is pulled from an internal project it makes sense for it to be 1.0.0-alpha as it has already been written.

I agree about 1.0.0 being stable and for production.

dextermb commented 5 years ago

v1.0.0 should be considered stable and usable in production, anything below v1.0.0 should be considered not production ready etc.

Using 0.0.1-alpha is fine for packages being built from the ground up with open source in mind. But if the code is pulled from an internal project it makes sense for it to be 1.0.0-alpha as it has already been written.

Thoughts on maybe carrying over the version or estimated version from personal projects being extracted to Langley Foxall?

I think you'd carry over the version that you believe your project to be at, if published for internal usage then append -alpha on the end- otherwise write documentation and apply company standards.

NilesB commented 5 years ago

@dextermb I agree with this.

AlexCatch commented 5 years ago

I think that when you need to extract a bit of code written in a project into it's own package - that's a new package so I'd start the versioning at v0.0.1-alpha otherwise it looks weird if a just released package has v1.2.1 etc.

Often you also have to refactor the code to make it suitable to work in a standalone package etc so it would again be pretty weird to go with what you think the package version is, it would almost look like you're pulling numbers out of thin air.

DivineOmega commented 5 years ago

My thoughts regarding this probably conflict with a lot of what has been said here.


For open source software development, I believe the following points should apply.


This allows rapid development of new packages, such that it does not block or impede development of related commercial projects, yet still allows for immediate usage and future improvement of those packages by anyone inside and outside of the company.

Obviously, even though I think we should push a v1.0.0 release as soon as possible for open source projects, we should aim to still push high quality code. If time permits and no delays to commercial projects would be caused, we should still aim to ensure style compliance, automated tests, and documentation are present in the first stable release, to at least some degree.

AlexCatch commented 5 years ago

@divineomega

  • Release Early, Release Often, a philosophy adopted by most of the open source community. Agree

  • Not use semantic versioning for v0.x.y releases.

I'd say we need to be consistent in our versioning, it would appear jagged if we don't follow semver from the get go & only decide to use it when we reach v1.0.0, I don't see a benefit in being inconsistent before a stable release

  • Once the package is functional and usable, release v1.0.0. This would be the only requirement, allowing us minimal barriers and delays to the initial package development and release.

I feel at a minimum we need to have tests written for the package to an adequate coverage level, if the code is going to be considered production ready, the only way to be sure that your code is 'functional and usable' to a high level is by having tests validate that, just like how we wouldn't push production code without having written tests for it in our commercial projects.

If it's being used in production, it has to have tests validating the functionality works, otherwise how can we be sure that this code we're pushing isn't breaking functionality of another developer's commercial project?

  • Semantic version to be strictly adhered to for releases >= v1.0.0.

Agree

  • Style compliance, automated tests, and documentation can be added later, if not present in the v1.0.0 release. This allows rapid development of new packages, such that it does not block or impede development of related commercial projects, yet still allows for immediate usage and future improvement of those packages by anyone inside and outside of the company.

I'm split - a v1.0.0 means it's ready for production, if it's going to be used in a production environment where the system relies on it to work 100% of the time, tests should be required to ensure the code works under different circumstances.

Style compliance should really be a fundamental aspect to the code we write and shouldn't be an afterthought although I agree it's not as important as if the code functions properly, it is important for maintainability though.

Documentation can be added at a later point though I agree.

Obviously, even though I think we should push a v1.0.0 release as soon as possible for open source projects, we should aim to still push high quality code. If time permits and no delays to commercial projects would be caused, we should still aim to ensure style compliance, automated tests, and documentation are present in the first stable release, to at least some degree.

Again part agree, style compliance, automated tests, and documentation should be a standard for all code we write, not an afterthought although I agree for a v1.0.0 release, documentation isn't strictly necessary.

I disagree with automated testing and style compliance (to an extent), automated testing ensures code fully works under the expected circumstances (without it how can we be sure that pushing this code won't break existing functionality in other developer's commercial projects?) & conforming to style guides ensure that the code is consistant and maintainable, a hallmark of production code (but not strictly necessary for proper function of the code)

DivineOmega commented 5 years ago
  • Not use semantic versioning for v0.x.y releases.

I'd say we need to be consistent in our versioning, it would appear jagged if we don't follow semver from the get go & only decide to use it when we reach v1.0.0, I don't see a benefit in being inconsistent before a stable release

I strongly agree with consistency in versioning, however I believe very early development prior to a package's general use should be excluded from this.

Semver dictates that major versions are used to indicate breaking changes. During very early development of a package, there are often several changes which could be considered breaking, such as usage syntax. If semver were followed at this stage, we may end up with packages at high major version numbers prior to them actually being functional for their intended use case(s).

In addition, a policy of pushing v1.0.0 as soon as possible helps mitigate any problems that may result from this.

Thoughts?

dextermb commented 5 years ago

I think for internal use when the package is within prototype stages, but being used in projects then the lack of documentation, linting and testing is fine. As soon as we put it on some sort of package host, be it composer or NPM, then it should be documented and adhere to our company standards unless marked as alpha or beta.

Again, versioning doesn't really matter until it goes out as you can install directly from GitHub rather than bumping versions for quick changes.

@DivineOmega @AlexCatch

AlexCatch commented 5 years ago

@DivineOmega I see your point, although often pre-release versioning is reset upon entering a stable release which mitigates this.

Often I'll mark my pre-release packages like v1.0.1-alpha to indicate this is v1.0.1 of alpha; when this moves to a stable release it's reset to v1.0.0. The semver version of the alpha or beta build can go as high as you want, e.g. v6.4.2-alpha but once it enters stable release it's reset to v1.0.0.

Alpha or beta probably isn't the right keyword here to use, I'd open for something like v1.0.0-pre-release. Just something to indicate this line of versioning isn't stable.

NilesB commented 5 years ago

Here's the NPM docs on this for reference.

dextermb commented 5 years ago

Perhaps people are talking about different prereleases. I believe that @AlexCatch is talking about a release candidate whereas everyone else is talking about internal usage.

But to reference the documentation, it says to use alpha.

If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it will only be allowed to satisfy comparator sets if at least one comparator with the same [major, minor, patch] tuple also has a prerelease tag.

... First, prerelease versions frequently are updated very quickly, and contain many breaking changes that are (by the author’s design) not yet fit for public consumption.

... Second, a user who has opted into using a prerelease version has clearly indicated the intent to use that specific set of alpha/beta/rc versions.

DivineOmega commented 5 years ago

@AlexCatch @dextermb It is worth noting that pre-release tag suffixes are redundant for Packagist, which automatically marks packages as stable or unstable, based on the version number and the pre-release GitHub flag.

Typically, I do not actually create v0.x.y releases and simply leave this on master until it is ready. The first release for the vast majority of the projects I work on is v1.0.0. This might be a policy we wish to adopt, which would immediately get rid of the majority of our versioning related issues.

AlexCatch commented 5 years ago

@DivineOmega I'm happy for this to be the case, stick to master until you reach v1.0.0 & then begin versioning once it's ready for production.

DivineOmega commented 5 years ago

@AlexCatch Agreed. πŸ‘

AlexCatch commented 5 years ago

@DivineOmega I'll get a PR done tonight and we'll go over it in more detail on that πŸŽ‰

dextermb commented 5 years ago

@AlexCatch I think you need to get feedback from more people, and be in agreement with more than one person before doing anything more πŸ‘

AlexCatch commented 5 years ago

@dextermb We can discuss all day, it's easier to get a PR done with what everyone who has commented has agreed on or has no objections to and go from there.

A PR is a start, otherwise nothing will get done πŸ‘

ash123456789 commented 5 years ago

Semantic Versioning I think we should follow semantic versioning more closely for the initial development phase of the package since SemVer is so widely used it would make sense to keep it consistent with what other developers are doing around the world - and in reality they should understand that 0.y.z is an unstable version that is not product ready. The README file should also statement the package is currently in development and not production ready. The SemVer documentation states the following, which I agree with.

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

StyleCI I believe StyleCI should be a base requirement since it takes little to no time to integrate with and we are already using it in multiple commercial projects with a general configuration already in place.

Documentation The README should contain, at a minimum, the following for a stable version.

Tests I agree with a lot of what has been so far and I think tests should only be a requirement for a 1.0.0 release.


One thing I have noticed (specifically on Packagist) is that some of us are naming our versions as v1.0.0 and others are using 1.0.0, I think this should be more consistent, I am personally in favour of 1.0.0 since the v is redundant.

AlexCatch commented 5 years ago

Semantic Versioning I think we should follow semantic versioning more closely for the initial development phase of the package since SemVer is so widely used it would make sense to keep it consistent with what other developers are doing around the world - and in reality they should understand that 0.y.z is an unstable version that is not product ready. The README file should also statement the package is currently in development and not production ready. The SemVer documentation states the following, which I agree with.

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

Agree, although only if you are releasing a version before v1.0.0, Eloquent CSV Importer is a great example of this, I had to release a version before it was stable to install it into one of our internal projects, notice it's marked as a pre-release as well.

StyleCI I believe StyleCI should be a base requirement since it takes little to no time to integrate with and we are already using it in multiple commercial projects with a general configuration already in place.

Part agree - yes for PHP, in relation to JS we'll need to see if it matches our internal JS style guide as I know we've personally had some issues and had to revert back to ESLint, I feel this would come in under more specific OSS requirements for different technologies & not generalised release requirements that can be applied to any technology.

Documentation The README should contain, at a minimum, the following for a stable version.

  • What the package is and what it's for.
  • How the package can be installed (usually composer).
  • Basic usage of the package and what features it contains.

Agreed.

Tests I agree with a lot of what has been so far and I think tests should only be a requirement for a 1.0.0 release.

Agreed.

One thing I have noticed (specifically on Packagist) is that some of us are naming our versions as v1.0.0 and others are using 1.0.0, I think this should be more consistent, I am personally in favour of 1.0.0 since the v is redundant.

Disagree, although it doesn't matter much, I prefer v1.0.0 just from an aesthetic view.

I use it mostly because it's used a lot across the industry etc.

https://github.com/facebook/react/releases https://github.com/laravel/laravel/releases https://github.com/vuejs/vue/releases

I also feel it's because releases also center around git tags which can be used for multiple purposes so specifying the v before the version makes it's clear that this tag indicates a new release and not another tag indicating something else.

DivineOmega commented 5 years ago

Semantic Versioning I think we should follow semantic versioning more closely for the initial development phase of the package since SemVer is so widely used it would make sense to keep it consistent with what other developers are doing around the world - and in reality they should understand that 0.y.z is an unstable version that is not product ready. The README file should also statement the package is currently in development and not production ready. The SemVer documentation states the following, which I agree with.

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

Agree, although only if you are releasing a version before v1.0.0, Eloquent CSV Importer is a great example of this, I had to release a version before it was stable to install it into one of our internal projects, notice it's marked as a pre-release as well.

@ash123456789 Interestingly, that section of the SemVer documentation essentially means SemVer does not apply for versions < v1.0.0. I'm also in agreement.

@AlexCatch Although it's perfectly fine to release version < v1.0.0, it is worth noting that Composer supports installing directly from the master branch by specifying dev-master as the version constraint. A release is technically not necessary during early development unless you need/wish to lock the development version the project is using.

StyleCI I believe StyleCI should be a base requirement since it takes little to no time to integrate with and we are already using it in multiple commercial projects with a general configuration already in place.

Part agree - yes for PHP, in relation to JS we'll need to see if it matches our internal JS style guide as I know we've personally had some issues and had to revert back to ESLint, I feel this would come in under more specific OSS requirements for different technologies & not generalised release requirements that can be applied to any technology.

@ash123456789 Agreed, assuming it will not impact commercial deadlines.

Documentation The README should contain, at a minimum, the following for a stable version.

  • What the package is and what it's for.
  • How the package can be installed (usually composer).
  • Basic usage of the package and what features it contains.

Agreed.

@ash123456789 That's a great outline of a basic package README. I tend to follow roughly that pattern, often including some badge and maybe a screenshot / graphical code snippet if appropriate.

Again, agreed, assuming it will not impact commercial deadlines.

Tests I agree with a lot of what has been so far and I think tests should only be a requirement for a 1.0.0 release.

Agreed.

One thing I have noticed (specifically on Packagist) is that some of us are naming our versions as v1.0.0 and others are using 1.0.0, I think this should be more consistent, I am personally in favour of 1.0.0 since the v is redundant.

Disagree, although it doesn't matter much, I prefer v1.0.0 just from an aesthetic view.

I use it mostly because it's used a lot across the industry etc.

https://github.com/facebook/react/releases https://github.com/laravel/laravel/releases https://github.com/vuejs/vue/releases

@ash123456789 @AlexCatch I also prefer the v prefix, it is very commonplace and in fact recommended by GitHub when creating new releases, as shown below.

Tagging suggestions

It’s common practice to prefix your version names with the letter v. Some good tag names might be v1.0 or v2.3.4.

DivineOmega commented 5 years ago

The level of quoting here is getting slightly absurd, so we may need to start splitting these points into separate issues.

AlexCatch commented 5 years ago

@DivineOmega I feel like if you're at a point where releasing an open source package will impact deadlines as you have to write tests, docs etc, don't release the package yet and use a pre release version in the commercial project, as long as you have adequate browser tests in the project itself to test the integrated package.

DivineOmega commented 5 years ago

@AlexCatch That seems reasonable, overall.

Though you would want the public API to be stable by the time it is used in a production project, meaning a v1.0.0 would generally be a good idea. SemVer documentation defines < v1.0.0 as being in an 'Anything may change at any time.' state.

AlexCatch commented 5 years ago

If you make a pre-release, something like v1.0.0-pre-release and install that in your project, that should never change & it's indicated that this isn't a full release by marking it as a so when creating the release in Github.

https://github.com/langleyfoxall/eloquent-csv-importer is a great example of that.

dextermb commented 5 years ago

After having a look around on publishing NPM packages it seems like there are dist-tags. According to the documentation:

For example, a project might choose to have multiple streams of development and use a different tag for each stream, e.g., stable, beta, dev, canary.

This is probably a better solution than suffixing versions. This also clears up why npm version <type> clears any suffixed versions.

Thoughts?

cc: @DivineOmega