Typescript + Tests + Namespaced npm package #7

kumavis · 2020-09-21T06:02:41Z

based on the metamask repo template, but halp test not passing

kumavis · 2020-09-21T08:34:28Z

rollup has so much trouble with readable-stream 🚒

kumavis · 2020-09-21T08:42:20Z

wait why is the main export a bundle

kumavis · 2020-09-21T09:09:36Z

dist tree getting a bit hairy

dist
├── BasePostMessageStream.d.ts
├── bundle.cjs.js
├── bundle.es.js
├── cjs
│   ├── BasePostMessageStream.d.ts
│   ├── BasePostMessageStream.js
│   ├── BasePostMessageStream.js.map
│   ├── enums.d.ts
│   ├── enums.js
│   ├── enums.js.map
│   ├── index.d.ts
│   ├── index.js
│   ├── index.js.map
│   ├── index.test.d.ts
│   ├── index.test.js
│   ├── index.test.js.map
│   ├── WindowPostMessageStream.d.ts
│   ├── WindowPostMessageStream.js
│   ├── WindowPostMessageStream.js.map
│   ├── WorkerParentPostMessageStream.d.ts
│   ├── WorkerParentPostMessageStream.js
│   ├── WorkerParentPostMessageStream.js.map
│   ├── WorkerPostMessageStream.d.ts
│   ├── WorkerPostMessageStream.js
│   └── WorkerPostMessageStream.js.map
├── enums.d.ts
├── esm
│   ├── BasePostMessageStream.d.ts
│   ├── BasePostMessageStream.js
│   ├── BasePostMessageStream.js.map
│   ├── enums.d.ts
│   ├── enums.js
│   ├── enums.js.map
│   ├── index.d.ts
│   ├── index.js
│   ├── index.js.map
│   ├── index.test.d.ts
│   ├── index.test.js
│   ├── index.test.js.map
│   ├── WindowPostMessageStream.d.ts
│   ├── WindowPostMessageStream.js
│   ├── WindowPostMessageStream.js.map
│   ├── WorkerParentPostMessageStream.d.ts
│   ├── WorkerParentPostMessageStream.js
│   ├── WorkerParentPostMessageStream.js.map
│   ├── WorkerPostMessageStream.d.ts
│   ├── WorkerPostMessageStream.js
│   └── WorkerPostMessageStream.js.map
├── index.d.ts
├── index.test.d.ts
├── WindowPostMessageStream.d.ts
├── WorkerParentPostMessageStream.d.ts
└── WorkerPostMessageStream.d.ts

kumavis · 2020-09-21T10:00:58Z

hell is empty and all the devils are here 👍

kumavis · 2020-09-22T07:09:43Z

maybe hoping this gets fixed here nodejs/readable-stream#348

danfinlay · 2020-09-22T22:58:53Z

When I npm test I just get No tests found, exiting with code 1.

kumavis · 2020-09-23T03:10:45Z

drop bundle (for now)
expose node env in workers nodeIntegrationInWorker: true

kumavis · 2020-09-23T07:00:13Z

why no ci

kumavis · 2020-09-23T07:05:50Z

CI avalanche lol

kumavis · 2020-09-23T07:46:07Z

cool, just needs coverage tweaks

whymarrh · 2020-09-23T18:12:09Z

I've created MetaMask/metamask-module-template#19 as an updated baseline for the CI workflow we're using here—once that's merged we can update this workflow to have a similar shape.

I've also created #8, #9, #10, and #11, factoring out some of the extra changes from this PR. We can rebase this and drop those files to keep this changeset focused.

whymarrh

A few initial comments—I'll check it out locally as well.

whymarrh · 2020-09-23T18:17:57Z

jest.config.js

+const merge = require('merge');
+const tsPreset = require('ts-jest/jest-preset');
+
+module.exports = merge.recursive(tsPreset, {


Manually merging our config with the preset, is that necessary? Looking at the configuration docs for ts-jest, the preset key can be used.^[1] Merging this manually also complicates this process as we both now have an extra dependency and a resolution process (i.e. how does merge.recursive work? how many levels does it do? how does it treat the arrays?) to manage.

If we can get away with using preset: 'ts-jest', let use that.

this came from an earlier point where there were two presets, so i had to merge the presets manually. not needed any more 👍

whymarrh · 2020-09-23T18:19:09Z

package.json

-  "repository": {
-    "type": "git",
-    "url": "git+https://github.com/kumavis/post-message-stream.git"
-  },


This file will need to be updated to keep some of the original metadata, that's still useful. I've updated it in #11.

whymarrh · 2020-09-23T18:21:11Z

package.json

-  },
-  "bugs": {
-    "url": "https://github.com/kumavis/post-message-stream/issues"
+    "@types/readable-stream": "^2.3.9",


Could we move this to the devDependencies and export Node.js types?

readable-stream is better because its semantically versioned, node ships breaking changes with its builtins
(if i understood)

whymarrh · 2020-09-23T18:24:26Z

tsconfig.json

+    "allowJs": true,
+    "allowSyntheticDefaultImports": true


Why is allowJs enabled? I noticed below we're also listing "./src/**/*.js" as included.

I figure this out locally, we're not currently reading these files. See #7 (comment) for context.

the js has been removed, this is no longer needed 👍

whymarrh · 2020-09-23T19:04:20Z

package.json

+    "build:compile": "yarn build:compile:esm && yarn build:compile:cjs",
+    "build:compile:esm": "tsc --module ES6 --outDir ./dist/esm",
+    "build:compile:cjs": "tsc --module UMD --outDir ./dist/cjs",


Looking at this locally, I'm not sure I follow what having two builds is for? Could we ship only the CommonJS version?

I'm also confused by the naming here, though maybe it's a typo: build:compile:cjs is outputting UMD to the /cjs? Should that be outputting CJS instead?

We could move these into tsconfig(.*).json files, e.g.:

@@ -21,8 +21,8 @@ "build": "yarn run build:clean && yarn run build:compile && yarn run build:bundle", "build:clean": "rm -rf ./dist && mkdir -p ./dist/esm && mkdir -p ./dist/cjs", "build:compile": "yarn build:compile:esm && yarn build:compile:cjs", - "build:compile:esm": "tsc --module ES6 --outDir ./dist/esm", - "build:compile:cjs": "tsc --module UMD --outDir ./dist/cjs", + "build:compile:esm": "tsc --project tsconfig.es2015.json", + "build:compile:cjs": "tsc --project tsconfig.json", "build:bundle": "browserify --standalone PostMessageStream ./dist/cjs/index.js > ./dist/bundle.js" }, "license": "MIT",

With two config files: tsconfig.json and tsconfig.es2015.json:

@@ -4,17 +4,15 @@ "declaration": true, "sourceMap": true, "strict": true, - "target": "es5", + "target": "ES5", + "module": "CommonJS", "lib": [ "DOM", "ES2016" ], - "outDir": "./dist", - "allowJs": true, - "allowSyntheticDefaultImports": true + "outDir": "./dist/cjs" }, "include": [ - "./src/**/*.ts", - "./src/**/*.js" + "./src/**/*.ts" ] -} \ No newline at end of file +}

@@ -0,0 +1,7 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "target": "ES2015", + "module": "ES2015" + } +}

i think we should build for both (cjs+esm), the ecosystem (node v14, agoric, etc) is trying to get everyone on to esm

this setup is intended to be a hybrid package https://2ality.com/2019/10/hybrid-npm-packages.html

umd includes cjs with little overhead, so figured why not. can go either way

i like moving the config into project files 👍

package.json

whymarrh · 2020-09-23T19:11:51Z

package.json

  "scripts": {
-    "test": "echo \"Error: no test specified\" && exit 1"
+    "test": "yarn build && yarn testOnly",
+    "testOnly": "jest --runInBand",


Do we need the --runInBand flag here?

Suggested change

"testOnly": "jest --runInBand",

"testOnly": "jest",

The tests seem to pass without it, and the docs suggest it is a niche use case:

--runInBand, -i Run all tests serially in the current process (rather than creating a worker pool of child processes that run tests). This is sometimes useful for debugging, but such use cases are pretty rare. [boolean]

this reduces threads to 1 and is recommended for CI (also allows debugging with ndb)

whymarrh · 2020-09-23T19:13:21Z

package.json

+    "jest": "^26.4.2",
+    "merge": "^1.2.1",
+    "ts-jest": "^26.3.0",
+    "ts-node": "^9.0.0",


We should be able to remove this from the list, I don't think we're using it anywhere

kumavis · 2020-09-24T01:20:49Z

where do types deps go 🤔 https://stackoverflow.com/questions/45176661/how-do-i-decide-whether-types-goes-into-dependencies-or-devdependencies

kumavis · 2020-09-24T01:24:59Z

… from-template3

kumavis · 2020-09-25T01:27:13Z

dan asked about compiling in parallel https://github.com/kimmobrunfeldt/concurrently

rekmarks · 2021-04-28T22:01:23Z

I converted this to draft as I'm going to split up the work over a few different PRs. I will likely use the tests written here.

rekmarks · 2021-04-28T22:01:50Z

Actually, I'll just close it.

kumavis marked this pull request as ready for review September 23, 2020 06:53

kumavis requested a review from rekmarks September 23, 2020 07:09

kumavis changed the title ~~wip typescript and tests~~ Typescript + Tests + Namespaced npm package Sep 23, 2020

whymarrh suggested changes Sep 23, 2020

View reviewed changes

kumavis requested a review from a team as a code owner September 24, 2020 00:04

kumavis closed this Sep 24, 2020

kumavis force-pushed the from-template branch from 10a6e73 to ac44225 Compare September 24, 2020 06:58

typescript and dual builds

f3298a2

kumavis reopened this Sep 24, 2020

kumavis added 3 commits September 24, 2020 15:09

Merge branch 'master' of github.com:MetaMask/post-message-stream into…

091774d

… from-template3

MIT license

735717b

deps - readd lint deps

8fa26d2

kumavis added 2 commits September 25, 2020 15:31

apply (+fix) metamask-yeoman-generators

fee1bac

meta - add missing allow-scripts package and config

413abf0

rekmarks marked this pull request as draft April 28, 2021 21:48

rekmarks closed this Apr 28, 2021

rekmarks mentioned this pull request May 2, 2021

Add unit tests #17

Merged

rekmarks deleted the from-template branch May 2, 2021 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript + Tests + Namespaced npm package #7

Typescript + Tests + Namespaced npm package #7

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020 •

edited

Loading

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020

kumavis commented Sep 22, 2020

danfinlay commented Sep 22, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

whymarrh commented Sep 23, 2020

whymarrh left a comment

whymarrh Sep 23, 2020

kumavis Sep 24, 2020

whymarrh Sep 23, 2020

whymarrh Sep 23, 2020

kumavis Sep 24, 2020

whymarrh Sep 23, 2020

whymarrh Sep 23, 2020

kumavis Sep 24, 2020

whymarrh Sep 23, 2020

kumavis Sep 24, 2020

whymarrh Sep 23, 2020

kumavis Sep 24, 2020

whymarrh Sep 23, 2020

kumavis commented Sep 24, 2020

kumavis commented Sep 24, 2020 •

edited

Loading

kumavis commented Sep 25, 2020 •

edited

Loading

rekmarks commented Apr 28, 2021

rekmarks commented Apr 28, 2021

Typescript + Tests + Namespaced npm package #7

Typescript + Tests + Namespaced npm package #7

Conversation

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020 • edited Loading

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020

kumavis commented Sep 21, 2020

kumavis commented Sep 22, 2020

danfinlay commented Sep 22, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

kumavis commented Sep 23, 2020

whymarrh commented Sep 23, 2020

whymarrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Sep 24, 2020

kumavis commented Sep 24, 2020 • edited Loading

kumavis commented Sep 25, 2020 • edited Loading

rekmarks commented Apr 28, 2021

rekmarks commented Apr 28, 2021

kumavis commented Sep 21, 2020 •

edited

Loading

kumavis commented Sep 24, 2020 •

edited

Loading

kumavis commented Sep 25, 2020 •

edited

Loading