Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: build types from source instead of generated files #40

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

achingbrain
Copy link
Member

This module copies everything into the dist folder then builds types from there. multiformats builds types from the source, then copies moves the types folder into dist. The latter seems more predictable and means things like mikeal/ipjs#14 don't trip it up.

This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.
@@ -11,8 +11,8 @@
"scripts": {
"build": "npm run build:js && npm run build:types",
"build:js": "ipjs build --tests --main && npm run build:copy",
"build:copy": "mkdir -p dist/examples/ && cp -a tsconfig.json *.js *.ts lib/ test/ dist/ && cp examples/*.* dist/examples/",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trailing slash meant that the contents of the test folder (etc) was copied into dist rather than the test folder itself, which I guess was not intentional? Removing it means you don't need the extra cp command to copy the examples folder afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By which I mean the copied contents are getting flattened in the dist dir and files can overwrite each other if they happen to have the same name so I think this was not intended.

With trailing slash:

$ ls -l dist
total 472
-rw-r--r--   1 alex  staff    554 16 Jul 08:59 LICENSE-APACHE
-rw-r--r--   1 alex  staff   1053 16 Jul 08:59 LICENSE-MIT
-rw-r--r--   1 alex  staff  29679 16 Jul 08:59 README.md
-rw-r--r--   1 alex  staff   1258 16 Jul 07:10 api.ts
-rw-r--r--   1 alex  staff    395 26 May 10:52 car-browser.js
-rw-r--r--   1 alex  staff    371 26 May 10:52 car.js
drwxr-xr-x   7 alex  staff    224 16 Jul 08:59 cjs
-rw-r--r--   1 alex  staff    809 26 May 10:52 coding.ts
-rw-r--r--   1 alex  staff   8008 16 Jul 07:10 common.js
-rw-r--r--   1 alex  staff   8056 16 Jul 07:10 decoder.js
-rw-r--r--   1 alex  staff   1653 26 May 10:52 encoder.js
drwxr-xr-x   8 alex  staff    256 16 Jul 08:59 esm
drwxr-xr-x   9 alex  staff    288 16 Jul 08:59 examples
-rw-r--r--   1 alex  staff    715 26 May 10:52 go.car
-rw-r--r--   1 alex  staff     49 16 Jul 08:59 index.js
-rw-r--r--   1 alex  staff     64 16 Jul 08:59 indexed-reader
-rw-r--r--   1 alex  staff    153 26 May 10:52 indexed-reader-browser.js
-rw-r--r--   1 alex  staff   6203 16 Jul 07:10 indexed-reader.js
-rw-r--r--   1 alex  staff     49 16 Jul 08:59 indexer
-rw-r--r--   1 alex  staff   4120 26 May 10:52 indexer.js
-rw-r--r--   1 alex  staff     50 16 Jul 08:59 iterator
-rw-r--r--   1 alex  staff   1956 26 May 10:52 iterator-channel.js
-rw-r--r--   1 alex  staff   9000 26 May 10:52 iterator.js
-rw-r--r--   1 alex  staff   2311 26 May 10:52 node-test-file-streams.js
-rw-r--r--   1 alex  staff   1371 26 May 10:52 node-test-indexed-reader.js
-rw-r--r--   1 alex  staff   3786 26 May 10:52 node-test-large.js
-rw-r--r--   1 alex  staff   2466 26 May 10:52 node-test-raw.js
-rw-r--r--   1 alex  staff   2692 26 May 10:52 node-test-updateroots.js
-rw-r--r--   1 alex  staff   4517 16 Jul 08:59 package.json
-rw-r--r--   1 alex  staff     56 16 Jul 08:59 reader
-rw-r--r--   1 alex  staff   5767 16 Jul 07:10 reader-browser.js
-rw-r--r--   1 alex  staff   2069 16 Jul 07:10 reader.js
-rw-r--r--   1 alex  staff   2578 16 Jul 07:10 test-errors.js
-rw-r--r--   1 alex  staff   1966 26 May 10:52 test-indexer.js
-rw-r--r--   1 alex  staff    831 26 May 10:52 test-interface.js
-rw-r--r--   1 alex  staff   2555 26 May 10:52 test-iterator.js
-rw-r--r--   1 alex  staff   2982 26 May 10:52 test-reader.js
-rw-r--r--   1 alex  staff   9726 26 May 10:52 test-writer.js
-rw-r--r--   1 alex  staff   1252 16 Jul 07:36 tsconfig.json
drwxr-xr-x  10 alex  staff    320 16 Jul 08:59 types
-rw-r--r--   1 alex  staff   4863 26 May 10:52 verify-store-reader.js
-rw-r--r--   1 alex  staff     56 16 Jul 08:59 writer
-rw-r--r--   1 alex  staff   8018 16 Jul 07:10 writer-browser.js
-rw-r--r--   1 alex  staff   3253 26 May 10:52 writer.js

Without trailing slash:

$ ls -l dist
total 176
-rw-r--r--   1 alex  staff    554 16 Jul 08:59 LICENSE-APACHE
-rw-r--r--   1 alex  staff   1053 16 Jul 08:59 LICENSE-MIT
-rw-r--r--   1 alex  staff  29679 16 Jul 08:59 README.md
-rw-r--r--   1 alex  staff   1258 16 Jul 07:10 api.ts
-rw-r--r--   1 alex  staff    395 26 May 10:52 car-browser.js
-rw-r--r--   1 alex  staff    371 26 May 10:52 car.js
drwxr-xr-x   7 alex  staff    224 16 Jul 08:59 cjs
drwxr-xr-x   8 alex  staff    256 16 Jul 08:59 esm
drwxr-xr-x   9 alex  staff    288 16 Jul 08:59 examples
-rw-r--r--   1 alex  staff     49 16 Jul 08:59 index.js
-rw-r--r--   1 alex  staff     64 16 Jul 08:59 indexed-reader
-rw-r--r--   1 alex  staff     49 16 Jul 08:59 indexer
-rw-r--r--   1 alex  staff     50 16 Jul 08:59 iterator
drwxr-xr-x  14 alex  staff    448 16 Jul 07:10 lib
-rw-r--r--   1 alex  staff   4515 16 Jul 08:59 package.json
-rw-r--r--   1 alex  staff     56 16 Jul 08:59 reader
drwxr-xr-x  16 alex  staff    512 16 Jul 08:24 test
-rw-r--r--   1 alex  staff   1252 16 Jul 07:36 tsconfig.json
drwxr-xr-x  10 alex  staff    320 16 Jul 08:59 types
-rw-r--r--   1 alex  staff     56 16 Jul 08:59 writer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only happens with bsd cp .. I mainly dev on Linux and also have coreutils installed on my mac machine so I never see (or have to think) about this behaviour! But consistency is good to avoid footguns, so all good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I had wondered if it might have been something like that since the published directory structure on npm is correct.

"build:copy": "mkdir -p dist/examples/ && cp -a tsconfig.json *.js *.ts lib/ test/ dist/ && cp examples/*.* dist/examples/",
"build:types": "npm run build:copy && cd dist && tsc --build",
"build:copy": "mkdir -p dist/examples/ && cp -a tsconfig.json *.js *.ts lib test examples dist/",
"build:types": "tsc --build && mv types dist",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to run the copy step twice if we're not building types from the dist folder.

package.json Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jul 16, 2021

Yeah, I guess we could flip this; you'll notice the same pattern everywhere we're using ipjs, including js-multiformats. We started doing this because the original pattern built types and put them into dist/types/ but you ended up with ../../foo.js links from those type mappings, which didn't work when you publish from within dist/. .. also, I prefer to publish all important assets, including examples and original sources and tests.

@achingbrain
Copy link
Member Author

the original pattern built types and put them into dist/types/ but you ended up with ../../foo.js

I guess that was the output dir in the tsconfig and it was just creating relative paths then?

also, I prefer to publish all important assets, including examples and original sources and tests.

Nothing's changed on that front 😄. This can get quite big so it's worth keeping an eye on.

@rvagg rvagg merged commit b2fb0e3 into master Jul 28, 2021
@rvagg rvagg deleted the fix/build-types-from-source branch July 28, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants