Build system changes #252

matheus23 · 2021-06-14T16:46:07Z

This PR's changes are mostly centered around the package.json file:

Switch from borc to cborg. This gives us more precise typescript types for cbor. (Here is why js-ipfs switched)
Remove rollup. It would only apply on the index.[es5/cjs/umd].js files and all other modules, e.g. fs/protocol/private/namefilter.js (I need that for account recovery) would be unchanged from the output from tsc. However, we can just remove rollup by:
- Adding "browser": { crypto: false } to signal to bundlers to ignore require("crypto") in dependencies (noble-ec25519 has these requires, but is careful not to actually use them in browser contexts)
- Making dependencies always import { Buffer } from 'buffer'.
- Creating our index.min.js (previously index.umd.js) file with something else
Introduce esbuild for creating a convenient browser bundle at dist/index.min.js. It's fast. Like, <1 second. (Rollup took >20 secs for just the index.umd.js)
Move library files to lib/. We keep dist/index.min.js, though. I think this distinction is nice.
Introduce export maps. import "webnative/fs/protocol/private/namefilter" is now possible instead of import "webnative/dist/fs/protocol/private/namefilter".
Switch to github actions from travis CI
Add some utilities for better logging of errors in puppeteer. We now get console errors and unhandled exceptions that happen in the browser into our jest output.
Don't transpile "newer" features like async/await during tsc. We require node version >=15, which supports newer features like that anyway.

Seems like a lot, but it's not actually that many changes in terms of lines. Most of the changes are ~700 removed lines from getting rid of rollup dependencies.

To do

(Apart from getting a review of this)
Test that this works with different bundlers:

Let me know if I should add something to this list.

* Switch to type: module * Add esbuild * Don't bundle es5. Only bundle cjs(?) * Explicitly refer to Buffer from the buffer module

cborg doesn't have a default import

instead of "dist/"

Thus we can make them be type: commonjs to not confuse tooling

This reverts commit c5396bb.

Also: * Revert type: module and add type: module package.json to src/ (copied to lib/) * Change configuration files (babel/gen-version) back to cjs, to not confuse tooling * Fix tests that called 'new' on fs.empty * Update keystore-idb to v0.14.2 for browser-friendly builds

icidasset

Looks great! Added a few questions.

package.json

src/package.json

tests/helpers/page.ts

tsconfig.json

package.json

matheus23 · 2021-06-17T22:12:50Z

There's some issues left to solve.

In webpack, the relative imports between our files don't work (e.g. when index.js requires util, it import "./util"s. But webpack only understands import "./util.js" with the .js suffix) https://stackoverflow.com/questions/62619058/appending-js-extension-on-relative-import-statements-during-typescript-compilat
In snowpack, it doesn't get that when in webnative, we've got browser: { crypto: false } set, that this should also affect the noble-25519 library. When I manually change that, it works. I've created a PR in that project: Add "browser" field to package.json paulmillr/noble-ed25519#24

So, this PR is waiting on those things.

Node (& some bundlers) require that file imports (not package imports) reference the file with the file extension.

matheus23 · 2021-06-18T12:20:42Z

Ignore point (2) above. Not sure why there was an issue, I must have done something wrong. It works flawlessly now.

I still need to fix browser tests. Not sure what's going on. :/

Oh and to solve (1), I've changed all our imports. Previously, our imports could only be resolved by typescript, esbuild and rollup (and maybe a couple more). But not e.g. browserify or node.js. This is because they don't support extension-less relative imports. Node doesn't understand import "./util". It needs import "./util.js".
Sorry that's a huge diff, but the only way of doing platform-independent es modules in typescript today.
And if that's not confusing enough, we can still import "webnative/utils" (without an extension) from outside webnative, or more specifically, because it's not a relative import.

Doesn't work either.

This reverts commit 6c774dc.

This reverts commit fa30fb8.

And add /index to some imports, because that resolution step isn't done in ttsc.

matheus23 · 2021-06-21T11:17:39Z

I've reverted changes from the last comment. I've followed another strategy instead.

So, to recap, the issue was: Different environments use different semantics for resolving imports.

In typescript it's valid to import a file util.ts in a relative import via import "./util". This is also true for some bundlers, e.g. esbuild and vite. However, if you've got a file utils.js, nodejs will not let you import "./util". In nodejs you need to add an extension.

According to the stackoverflow post I linked above, most people just use import "./util.js" in typescript to refer to both util.ts (in the source files) and util.js in the compiled files in the end. Typescript allows you to use .js extensions to refer to .ts files (which is a result of this typescript issue).

However, unfortunately that won't work with jest. It uses babel and some typescript plugin to let you write your tests in typescript and it doesn't 100% match the typescript import semantics. It doesn't allow relative .js imports to refer to .ts files. Thus, changing all imports to .js doesn't work. I briefly tried to use ts-jest instead, but that didn't work out either.

So, I followed the same strategy as this PR. They basically have 100% the same problems as we had.
We can transform the tsc output by using "transform typescript", which is the ttypescript package and ttsc CLI. It allows us to install a plugin that adds .js to imports in the transpiled .js output.

One thing that it doesn't do: It doesn't correctly resolve /index.js imports. So if you have, say util/index.ts and you try to import "./util", it won't work. Instead you have to import "util/index" and it'll get transformed to import "util/index.js", which resolves fine.

So much for the explanation. Tests are now running and it's working with various environments: vite, snowpack, esbuild, webpack 5, and - as soon as we'll expose the dependency injection stuff - it'll work in nodejs.

walkah

Thanks for cleaning all this stuff up!

walkah · 2021-06-22T15:02:09Z

package.json

+    "start": "ttsc -w",
    "test": "jest --forceExit",


Not totally relevant, but do we want to switch stuff around so yarn test works "out of the box"? I would suggest make it the same as test:unit and drop test:unit ... maybe test:prod should be test:integration to be more accurate?

icidasset · 2021-06-22T15:05:16Z

Took a look at the latest changes, looks good 👍 Bit of a shame we have to move away from the dir/index.ts pattern, but I guess we can find a better alternative when do the next big refactor.

matheus23 · 2021-06-22T15:18:22Z

Bit of a shame we have to move away from the dir/index.ts pattern

Yes. 😕
We could change all x/index.ts files to be x.ts instead. 🤔

I'll keep in mind for the next refactor.

matheus23 added 30 commits June 10, 2021 18:13

Checkpoint:

2afe22f

* Switch to type: module * Add esbuild * Don't bundle es5. Only bundle cjs(?) * Explicitly refer to Buffer from the buffer module

Switch to cborg

b6d5813

Fix cborg imports

457fded

cborg doesn't have a default import

Only use tsc & esbuild for building & remove rollup

8e50ee0

Clarify node version requirement

25031f8

Fix README

927a84d

Remove prettier & unused npm dependencies

96c9c06

Actually remove rollup file lol

2a0e237

Fix build by using typedoc@beta (supports ts 4.3)

357ac15

Use the npm prepare script

b0c8810

Target es2020, build output to "modules/"

c84d461

instead of "dist/"

Try to put config files into config/

c5396bb

Thus we can make them be type: commonjs to not confuse tooling

Revert "Try to put config files into config/"

37bb7eb

This reverts commit c5396bb.

Setup github actions, remove travis

872a156

Try using setup-node instead of nix

51dd228

Fix typo

b8c1a60

Goddamnit I need type checking

e78d919

Try the network-concurrency flag

8b038f5

Debug

7e9118e

Try running tests with nix instead

1d8803c

Better error for loadWebnativePage

c0c5829

Try something

a27e013

Let's try something weird

5bf0d2c

Try waiting for domcontentloaded

edf3f47

Try writing webnative to the window object

19be18c

Try setup-node again

c60a2d7

Try waiting for networkidle2

5b9c3e8

Output errors happening in puppeteer to console

1be0f36

Bump fission-bloom-filters version

ab3e2d6

matheus23 marked this pull request as ready for review June 15, 2021 19:18

icidasset approved these changes Jun 16, 2021

View reviewed changes

package.json Show resolved Hide resolved

package.json Show resolved Hide resolved

src/package.json Show resolved Hide resolved

tests/helpers/page.ts Show resolved Hide resolved

icidasset reviewed Jun 16, 2021

View reviewed changes

tsconfig.json Show resolved Hide resolved

icidasset reviewed Jun 16, 2021

View reviewed changes

package.json Show resolved Hide resolved

Also export dist directory

66635fc

Add file extension to imports/exports

fa30fb8

Node (& some bundlers) require that file imports (not package imports) reference the file with the file extension.

matheus23 added 4 commits June 18, 2021 21:11

Try using ts-node for tests

6c774dc

Doesn't work either.

Revert "Try using ts-node for tests"

b38286f

This reverts commit 6c774dc.

Revert "Add file extension to imports/exports"

787786f

This reverts commit fa30fb8.

Use transform typescript to add .js to generated imports

518f6a0

And add /index to some imports, because that resolution step isn't done in ttsc.

matheus23 added 2 commits June 21, 2021 13:58

Add more missing /index imports

cbb2fec

Update noble-ed25519 with better browser support

48bb593

walkah approved these changes Jun 22, 2021

View reviewed changes

matheus23 merged commit 212d132 into main Jun 22, 2021

matheus23 deleted the matheus23/build-system branch June 22, 2021 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Build system changes #252

Build system changes #252

Uh oh!

matheus23 commented Jun 14, 2021 •

edited

Loading

Uh oh!

icidasset left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matheus23 commented Jun 17, 2021

Uh oh!

matheus23 commented Jun 18, 2021 •

edited

Loading

Uh oh!

matheus23 commented Jun 21, 2021

Uh oh!

walkah left a comment

Uh oh!

walkah Jun 22, 2021

Uh oh!

icidasset commented Jun 22, 2021

Uh oh!

matheus23 commented Jun 22, 2021 •

edited

Loading

Uh oh!

Uh oh!

Build system changes #252

Build system changes #252

Uh oh!

Conversation

matheus23 commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Uh oh!

icidasset left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matheus23 commented Jun 17, 2021

Uh oh!

matheus23 commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matheus23 commented Jun 21, 2021

Uh oh!

walkah left a comment

Choose a reason for hiding this comment

Uh oh!

walkah Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

icidasset commented Jun 22, 2021

Uh oh!

matheus23 commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matheus23 commented Jun 14, 2021 •

edited

Loading

matheus23 commented Jun 18, 2021 •

edited

Loading

matheus23 commented Jun 22, 2021 •

edited

Loading