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

feat: use aegir for build/testing/release #196

Closed
wants to merge 6 commits into from
Closed

Conversation

RangerMauve
Copy link
Collaborator

This is part of the overall ESM migration. ipld/ipld#224

We're removing building cjs on publish and are replacing a bunch of our existing build tooling to publish just ESM with typescript declarations using aegir

TODO:

  • Check that release scripts will still work
  • Update github actions with new changes (standardize actions accross repos?)

@RangerMauve RangerMauve requested a review from rvagg August 26, 2022 23:02
yarn-error.log Outdated Show resolved Hide resolved
@RangerMauve
Copy link
Collaborator Author

I'm getting the following errors when I try to lint:

/home/mauve/programming/js-multiformats/src/block.js:25:11: Blocks are nested too deeply (5). Maximum allowed is 4. [Error/max-depth]
/home/mauve/programming/js-multiformats/src/block.js:59:11: Blocks are nested too deeply (5). Maximum allowed is 4. [Error/max-depth]
/home/mauve/programming/js-multiformats/test/test-cid.spec.js:168:5: Unexpected 'todo' comment: 'TODO: after i have a keccak hash for the...'. [Warning/no-warning-comments]
/home/mauve/programming/js-multiformats/test/ts-use/src/main.ts:0:0: Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: test/ts-use/src/main.ts.
The file must be included in at least one of the projects provided. [Error]

Any tips for what I should do about the ts-use section?

For the "blocks are nested too deeply" bit, I was thinking of splitting out some code into separate functions, but the use of yield makes it harder. Should I just disable the rule for that file?

What should I do about the TODO comment?

@RangerMauve
Copy link
Collaborator Author

TODO: Compare the types published from aegir with old data.

@rvagg
Copy link
Member

rvagg commented Sep 6, 2022

such a hard pill to swallow, and one of the primary reasons I've resisted aegir to date:

added 478 packages, and audited 479 packages in 11s

$ du -sh node_modules/
162M    node_modules/

which is bad enough

to:

added 1402 packages, and audited 1613 packages in 1m

$ du -sh node_modules/
367M    node_modules/

rvagg and others added 2 commits September 6, 2022 13:18
## [9.8.0](v9.7.1...v9.8.0) (2022-09-06)

### Features

* use aegir for build/testing/release ([10d4604](10d4604))

### Bug Fixes

* address JS & TS linting complaints ([e494e3c](e494e3c))
@rvagg
Copy link
Member

rvagg commented Sep 6, 2022

Fixed up the linting complaints and pushed to this branch, @RangerMauve you may want to pull rebase for this.

A quick look at the types differences:

  • Lots of replacements for Object to `object
  • Lots of minor comment linting
  • BlockEncoder's encode goes from encode(data: T): ByteView<T>; to encode: (data: T) => ByteView<T>;
  • BlockDecoder's decode goes from decode(bytes: ByteView<T>): T; to decode: (bytes: ByteView<T>) => T;
  • MultihashHasher's digest goes from digest(input: Uint8Array): Promise<MultihashDigest> | MultihashDigest; to digest: (input: Uint8Array) => Promise<MultihashDigest> | MultihashDigest;
  • SyncMultihashHasher's digest goes from digest(input: Uint8Array): MultihashDigest; to digest: (input: Uint8Array) => MultihashDigest;

I think those method definition changes are fine. The Object -> object thing might be worth digging in to further since I think that's a narrowing of the types that can be fed in as arguments.

Also, pushing to this branch and making the tests pass resulted in a release! So something's wonky and I'll need to push a patch release to overwrite it.

@rvagg
Copy link
Member

rvagg commented Sep 6, 2022

Fixed up the release thing, there was an error in the release block in Actions:

diff --git a/.github/workflows/js-test-and-release.yml b/.github/workflows/js-test-and-release.yml
index 4c19afa..76f568f 100644
--- a/.github/workflows/js-test-and-release.yml
+++ b/.github/workflows/js-test-and-release.yml
@@ -126,7 +126,7 @@ jobs:
   release:
     needs: [test-node, test-chrome, test-chrome-webworker, test-firefox, test-firefox-webworker, test-electron-main, test-electron-renderer]
     runs-on: ubuntu-latest
-    if: github.event_name == 'push' && github.ref == 'refs/heads/esm-migration' # with #262 - 'refs/heads/${{{ github.default_branch }}}'
+    if: github.event_name == 'push' && github.ref == 'refs/heads/${{{ github.default_branch }}}'
     steps:
       - uses: actions/checkout@v3
         with:

I've pushed a 9.8.1 to restore the 9.7.1 code.

@rvagg
Copy link
Member

rvagg commented Sep 6, 2022

So, a couple of things to note as I drive by this:

  • npm test doesn't lint for me, I have to run this separately. Is this the pattern the other aegir projects are taking? I personally quite like having lint & test bundled in to one but am happy to stick to pattern if that's how it needs to be.
  • When we briefly had the 9.8.0 in the wild, it came back in as a dependency for master (because a few packages used for test pull in multiformats using ^ semver ranges) but I was getting loader errors about not finding the ./basics export when trying to run npm test. It could just be the way things were arranged but seemed suspicious to me because I think npm test should be doing ESM imports and the package.json here has a "./basics" which has an "import", so I'm scratching my head about that one and wondering if it's going to be a problem when we publish. Maybe a pre-release before we push this out would be a good thing to do so we can properly test integration with other packages instead of just simulating it (although simulating would certainly be a good idea too).
  • The .github/workflows/js-test-and-release.yml seems to suggest that tests are only going to be run on ${{{ github.default_branch }}} (when the hardwired esm-migration is removed)—is this correct? Shoulnd't we be running them on all branches? There's already a clause (fixed above) for not doing release unless it's on master so that shouldn't be a problem.

I'm going to try and get #197 and this one bundled together for a v10.0.0 release; somehow, still think through that process.

@rvagg rvagg mentioned this pull request Sep 6, 2022
5 tasks
@rvagg
Copy link
Member

rvagg commented Sep 6, 2022

OK, I've "merged" this with #197 into a proposal/v10.0.0 branch, PR @ #199. So I'll close this one and we can PR incremental changes against proposal/v10.0.0.

I just have a weird and annoying browser failure now, it doesn't seem to be reading the "browser" export for hashes/sha2 but I'm not sure what would have stopped that from working. It works on this branch but not on proposal/v10.0.0.

@rvagg rvagg closed this Sep 6, 2022
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.

3 participants