Redo repo #42

Merged
merged 15 commits into from Jan 13, 2017

Projects

None yet

4 participants

@themadcreator
Collaborator

This commit is a major refactoring and cleanup of this library.

  • Conversion of namespaces to modules while leaving the toplevel
    API intact.
  • Simplified building and testing to npm scripts only.
    • tsc for compilation
    • mochify for testing
    • typedoc for docs
  • Removed standalone library file and support for bower.
  • Adding typescript assets and compiled typescript with types and
    sourcemaps to enable downstream users to import fully typed source.
themadcreator added some commits Jan 10, 2017
@themadcreator themadcreator Redo repo
This commit is a major refactoring and cleanup of this library.

* Conversion of namespaces to modules while leaving the toplevel
  API intact.
* Simplified building and testing to npm scripts only.
    ** tsc for compilation
    ** mochify for testing
    ** typedoc for docs
* Removed standalone library file and support for bower.
* Adding typescript assets and compiled typescript with types and
  sourcemaps to enable downstream users to import fully typed source.
c501c2b
@themadcreator themadcreator Adding tslint rules from blueprint and fixing lint errors
d662648
@themadcreator themadcreator Adding coverage checks aef0219
@themadcreator themadcreator Removing unnecessary nesting of tests directories. Fixing coverage aaaeaf2
@themadcreator themadcreator Add license header to all source files
6c2dc77
@hellochar

Aweosme, lots of good work here, this is definitely a much needed refresh! Some questions:

a) We've been talking about removing this repo entirely and merging svg-typewriter code into Plottable directly. Are we still going to do that after this?
b) Removing the built file is kinda scary since various jsfiddles/random sites online could point to it directly. How much work would it be to not break these folks?
c) How does Plottable get these changes?
d) Any learnings from doing this that we can take with us into improving Plottable?

More detailed/specific review coming up

@hellochar

I'd like us to have a better release process (e.g. similar to either how Plottable or blueprint-chart does it)

- "typings": "svgtypewriter.d.ts",
+ "version": "1.0.0",
+ "description": "SVG Text Measurement Line Wrapper",
+ "main": "build/src/index.js",
@hellochar
hellochar Jan 10, 2017 Collaborator

The build folder is gitignored; will it exist when we publish to npm? How is publishing done now?

@themadcreator
themadcreator Jan 11, 2017 Collaborator

so i think the build dir is gitignored but NOT npmignored. I think that means it'll be included when we run npm publish. Haven't tried that yet though

@hellochar
hellochar Jan 11, 2017 Collaborator

Okay, we'll have to make sure we run yarn build or something before npm publish right?

- "tslint": "~3.7.4",
- "typescript": "~1.8.9"
+ "build": "npm-run-all build:clean build:compile build:namespace",
+ "build:clean": "rimraf ${npm_package_buildPath}",
@hellochar
hellochar Jan 10, 2017 Collaborator

why is rimraf better than rm -rf? Also, can you explain the ${npm_package_buildPath} syntax

@adidahiya
adidahiya Jan 10, 2017 Member

rimraf is platform-agnostic

+ "build": "npm-run-all build:clean build:compile build:namespace",
+ "build:clean": "rimraf ${npm_package_buildPath}",
+ "build:compile": "tsc --project .",
+ "build:namespace": "echo \"\\nexport as namespace SVGTypewriter;\" >> ${npm_package_types}",
@hellochar
hellochar Jan 10, 2017 Collaborator

whoa what is export as namespace? Is this some crazy Typescript magic that converts a module into a namespace?!

package.json
"dependencies": {
- "d3": "^3.5.16"
+ "@types/d3": "3.5.36",
+ "@types/es6-shim": "^0.31.32",
@hellochar
hellochar Jan 10, 2017 Collaborator

I'd peg @types versions to a specific version since they seem to change and break all the time

package.json
- "d3": "^3.5.16"
+ "@types/d3": "3.5.36",
+ "@types/es6-shim": "^0.31.32",
+ "d3": "3.5.16"
@hellochar
hellochar Jan 10, 2017 Collaborator

On the other hand, we shouldn't peg our actual code dependencies to a specific version. Is there a reason why we specifically need d3 3.5.16 vs any other version? I think ^3.5.0 is good

@@ -0,0 +1,9 @@
+/**
@hellochar
hellochar Jan 10, 2017 Collaborator

What are the big pros/cons of re-exporting in index vs. consumers doing

import * as BaseAnimator from "./animators/baseAnimator"

@adidahiya
adidahiya Jan 10, 2017 Member

re-exporting is better; it allows you to reduce your API surface area. if consumers have to know about the folder structure to import symbols, then that becomes part of your public API.

test/domTests.ts
+ * license at https://github.com/palantir/svg-typewriter/blob/develop/LICENSE
+ */
+
+/// <reference types="mocha"/>
@hellochar
hellochar Jan 10, 2017 edited Collaborator

our tests still need <reference> comments?

@themadcreator
themadcreator Jan 10, 2017 Collaborator

Turns out no, will remove

- ]);
-
- var travisTests = ["test"];
- if (process.env.SAUCE_USERNAME) {
@hellochar
hellochar Jan 10, 2017 Collaborator

Is this logic (saucelabs testing) replicated?

- ]);
-
- grunt.registerTask("default", "launch");
- grunt.registerTask("launch", ["connect", "dev-compile", "watch"]);
@hellochar
hellochar Jan 10, 2017 Collaborator

Looks like grunt had a way to spin up a webserver w/ livereload and watch files for changes, but I don't see a way to do this in the new repo?

@hellochar

Would like clarification on publishing and relaxing the dep version ranges

@@ -0,0 +1,9 @@
+/**
@adidahiya
adidahiya Jan 10, 2017 Member

re-exporting is better; it allows you to reduce your API surface area. if consumers have to know about the folder structure to import symbols, then that becomes part of your public API.

@@ -1,3 +1,9 @@
+/**
+ * Copyright 2017-present Palantir Technologies, Inc. All rights reserved.
@adidahiya
adidahiya Jan 10, 2017 Member

(probably a separate PR) you could use the file-header lint rule to enforce this

@themadcreator
themadcreator Jan 11, 2017 Collaborator

I went ahead and added that. Pretty simple

@themadcreator themadcreator Go full module mode. Update typedoc generation
e68faee
@themadcreator
Collaborator

@hellochar fair question about the lack of compiled asset and lack of jsfiddle support. I'm trying to avoid big build tools like grunt/gulp, so maybe just a browserify script would do the trick. Not sure if it'd bring the typings along properly, but worth investigating.

@hellochar
Collaborator

Hmm yeah it's probably fine to remove the compiled asset; we're dropping Bower support anyways.

.npmignore
test/
coverage/
circle.yml
tslint.json
+tsconfig.json
@hellochar
hellochar Jan 11, 2017 Collaborator

nit: I prefer how blueprint-chart has a yarn dist command that copies the relevant stuff into a dist/ folder; that way we whitelist the things that get packaged

themadcreator added some commits Jan 12, 2017
@themadcreator themadcreator Add dev preview. Fix whitespace issue.
Now you can use the compile-and-watch dev preview by running `npm start`.

There was an issue where strings ending in whitespace where not
measured correctly because an SVG text element with "AA" has
the same bounding box as "AA    ". To circumvent this issue,
we use the built-in guard strings when the measured text
ends in whitespace.
8ec94d4
@themadcreator themadcreator Adding docs and dev preview to commits and PRs
3aa6f12
@themadcreator themadcreator Fixing scripts
e28a4a6
@blueprint-bot

Fixing scripts

Preview: docs | dev

@themadcreator themadcreator Add artifacts to make previews visible
d25174f
@blueprint-bot

Add artifacts to make previews visible

Preview: docs | dev

@themadcreator themadcreator Fix title of dev preview page
a6dd66f
@blueprint-bot

Fix title of dev preview page

Preview: docs | dev

@adidahiya
Member

For a follow-up PR: we don't need to test on old versions of Node in CircleCI. I suggest matching TSLint: https://github.com/palantir/tslint/blob/74db5f9e668725d891f67ed39c4dedd506a01f75/circle.yml#L7

themadcreator added some commits Jan 12, 2017
@themadcreator themadcreator Use modern node versions. Add saucelabs crossbrowser tests
b51dfdc
@themadcreator themadcreator Kick build
70400b9
@themadcreator themadcreator Test command syntax
9c71190
@blueprint-bot

Test command syntax

Preview: docs | dev

@hellochar

This looks awesome!! Live previews and publishing from CI are huge improvements

.npmignore
@@ -2,7 +2,8 @@ docs/
test/
preview/
coverage/
-circle.yml
@hellochar
hellochar Jan 13, 2017 Collaborator

why do we now include these?

@adidahiya
adidahiya Jan 13, 2017 edited Member

all *.yml files are now ignored.

it would be safer to write this file like https://github.com/palantir/plottable/blob/develop/.npmignore, which whitelists files in the package rather than blacklists them, but we don't do that consistently everywhere (blueprint doesn't do it)

@themadcreator
themadcreator Jan 13, 2017 Collaborator

oh nice

@adidahiya
Member

I'm reviewing this code by looking at the branch https://github.com/palantir/svg-typewriter/blob/bd/redo/package.json

Please remove @types/es6-shim as a dependency in package.json. That's not the only way to get es6 types (see palantir/blueprint#475 (comment)), and including that line will cause real breakage for end users due to duplicate typings.

Also use a floating dependency range, ^3.5 for @types/d3.

@adidahiya
Member

Other than that, this looks good to go.

Minor: we usually use chokidar to create watch scripts, but I'd be curious to see how onchange works out for you. 👍

@adidahiya

see above comments

@themadcreator
Collaborator

I've used onchange for other projects and it works great

@themadcreator themadcreator Use npmignore whitelist. Change d3 types versions. Remove es6-shim
0fd2405
@blueprint-bot

Use npmignore whitelist. Change d3 types versions. Remove es6-shim

Preview: docs | dev

@themadcreator themadcreator merged commit 45c2bb5 into develop Jan 13, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/palantir CLA signed via membership in "palantir"
Details
@themadcreator themadcreator deleted the bd/redo branch Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment