Initial rough dev guidelines (visgl#679)

aravindiiitb · Jun 2, 2017 · faa3ff9 · faa3ff9
1 parent e5fe18f
commit faa3ff9
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 0 deletions.
diff --git a/dev-guidelines/README.md b/dev-guidelines/README.md
@@ -0,0 +1,19 @@
+# Development Guidelines
+
+This directory contains development guidelines for deck.gl.
+
+Note that these documents are also referenced by other repositories in the stack of open source components that deck.gl belongs to:
+* react-map-gl
+* luma.gl
+
+
+## Common
+
+The documents in this folder apply to all the repositories mentioned above:
+
+* [DEPRECATION-GUIDELINES](./common/deprecation-guidelines.md)
+* [TEST-GUIDELINES](./common/deprecation-guidelines.md)
+
+## deck.gl Specific Guidelines
+
+* [API-GUIDELINES](./deck.gl/API-GUIDELINES.md) - API audit guidelines for deck.gl
diff --git a/dev-guidelines/common/DEPRECATION-GUIDELINES.md b/dev-guidelines/common/DEPRECATION-GUIDELINES.md
@@ -0,0 +1,30 @@
+# DEPRECATION GUIDELINES
+
+Every feature removal should be given careful thought. Gently deprecating things (keep supporting them for 1+ release with a console warning) has a big value and is the preference.
+
+* But sometimes the price in e.g. complexity or code overhead can be too big (thinking about deck.gl v4 which has three big deprecated ChoroplethLayers that take up a lot of space in the app bundle but are used by very few apps. One could question whether keeping them was the right choice). The situation is similar for the less frequently used Overlays in react-map-gl v3.
+
+* In cases where we can't or won't do gentle deprecation, we should document the breaking changes in an upgrade guide.
+
+* I'm in favor of following what Facebook does with React in general - console.warn for API deprecations and clearly stating in the CHANGELOG and then deprecate 2-3 versions later.
+
+* The question also is when to eventually do the deprecation. Do we do it in a minor patch, or wait for a major version bump?
+
+ * iven we already have a few majors, everything that is potentially breaking should be done in a major
+ @ibgreen
+
+* Isn't the key consideration that we should provide some reasonable time for users to update? If a feature has been deprecated for 6+ months, and we are cutting a new major release, it would seem acceptable to remove it.
+
+* If it was just deprecated a week earlier in a minor release, and we are cutting a new major release, then we should probably keep it around as deprecated for another major release.
+
+* Even if it has only been deprecated for a week and we are about to draft a new major, we should take the opportunity to remove it completely as soon as possible. If users want to update to the latest major and stay 
+* gently deprecate with console.warn when possible (and not too burdensome)
+
+
+* note API deprecation and removal in CHANGELOG
+* keep deprecated APIs for one major release
+* remove deprecated APIs only on major versions, but don't be shy about it
+
+* Sounds good. But yeah timeline should be sensible. If we're cutting major releases every 2 weeks then we should also re-evaluate life a bit :p.
+
+* It'll help to keep a clear log, going forward, of planned API deprecation and when the warnings were introduced to the user so as we work on new major versions, we can have a clear sense of how much time the user has had to make the necessary changes.
diff --git a/dev-guidelines/common/TEST-GUIDELINES.md b/dev-guidelines/common/TEST-GUIDELINES.md
@@ -0,0 +1,46 @@
+# Test Setup Explained
+
+## Test Suite
+
+Using `tape`.
+
+
+## Node tests
+
+
+Idea behind `test/node.js` and `test/node-dist.js`
+
+These are supposed to work identically. The only difference being that one runs the test directly on the ES6 source and one runs the tests on the transpiled ES5 version of the library, just to make doubly sure the transpilation doesn't cause any issues. We should absolutely not import jsdom in just one of these two tests, they should both have the same setup, either neither or both should import it. Second, about jsdom usage in node tests:
+
+The big problem is that the introduction of jsdom hides an important class of problems - i.e. we will no longer catch issues that happen under node in apps that don't include jsdom - which are very common (stray references to window and document etc). Thus, importing jsdom defeats an important part of the purpose of the node tests.
+I suspect the jsdom import into the node test script was introduced during the push to increase coverage, which also saw the addition of the sinon dependency.
+I'm not sure if the reason jsdom was included was because sinon requires it, or if it was due to another issue.
+Regardless, my strong preference would be that we find a way to avoid including jsdom in node tests. If it is indeed sinon that requires it, we might be able to replace sinon with our own spy function. I don't think we are using much of that library.
+If we absolutely need to include jsdom we might need to create a third node test script for node (test/node-no-jsdom.js?) that runs at least some tests without jsdom included.
+Finally, in regards to the question about what tests do we want to run on test, pre-commit, travis CI, publish etc:
+
+For pre-commit, CI, publish, we want to run everything we got: the linter, run an error-free build, run multiple tests on node etc. Basically, we want to cast the widest possible net to prevent bugs from making their way into master and into the published packages.
+That said, for quick iteration of tests during development, it will be frustrating if too many tests are run each time. Since making sure that e.g. CI etc use all tests typically requires adding all the tests to the main package.json "test" script, in many modules we add a separate test-fast script that just runs one copy of the tests:
+
+```
+   "scripts": {
+      "test"; "npm run lint && node test/node.js && node test/node-dist.js && .... "
+      "test-fast" "node test/node.js",
+      ...
+   }
+
+   ```
+PS - We could possibly relax the pre-commit testing just a little if found too time consuming, but there is no reason to reduce the CI or prepublish testing.
+
+
+## npm run test-browser
+
+It's the same test suite, but run in the browser.
+
+While it would be awesome to have CI do both node and browser testing, I haven't found a reliable way to run it on Travis, so it doesn't get executed by the main test script.
+
+But since I find debugging in the browser much easier than debugging in node, I try to keep the script in good working order.
+
+But since it is not automatically run, it doesn't get tested before every check-in, so it does occasionally degrade on master.
+
+We did have a partially working integration with electron (headless browser environment) and testron, but it was flaky. If we could get something like that solid (perhaps Chrome headless mode), it would be good to add it to the main set of tests.
diff --git a/API-GUIDELINES.md → dev-guidelines/deck.gl/API-GUIDELINES.md b/API-GUIDELINES.md → dev-guidelines/deck.gl/API-GUIDELINES.md