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: fix repo to use new aegir #89

Merged
merged 24 commits into from
Feb 24, 2021
Merged

Conversation

hugomrdias
Copy link
Contributor

@hugomrdias hugomrdias commented Jan 26, 2021

  • fix: ts and lint errors
  • chore: add docs to gitignore
  • fix: fix setup for new aegir …
    • tweak tsconfig.js
    • add ts type check including test files
    • fix ci setup for new aegir
    • add new awesome aegir docs feature
      • you can publish to github pages with the flag --publish
  • aegir now copies .d.ts to dist

/cc @vasco-santos

needs:
ipfs/aegir#730

  • add back the cache after aegir release

- tweak tsconfig.js
- add ts type check including test files
- fix ci setup for new aegir
- add new awesome aegir docs feature
  - you can publish to github pages with the flag --publish
vasco-santos
vasco-santos previously approved these changes Jan 27, 2021
Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @hugomrdias

@mpetrunic the CI job seems stalled. Can you restart it to check if everything is working?

@mpetrunic
Copy link
Member

LGTM! Thanks @hugomrdias

@mpetrunic the CI job seems stalled. Can you restart it to check if everything is working?

We ran out of free minutes on travis. We are actually paying and using github actions. I think it's just one time thing that we used our free minutes on travis since we had some fork running it's actions there.

Do we wait 4 more days to reset? 😄

@vasco-santos
Copy link
Collaborator

Do we wait 4 more days to reset? 😄

We are also moving to github actions. So perhaps we can just change that here?

@mpetrunic
Copy link
Member

Do we wait 4 more days to reset? smile

We are also moving to github actions. So perhaps we can just change that here?

not sure If I can push to this branch but I'll change ci right now

@hugomrdias
Copy link
Contributor Author

if it helps
https://github.com/ipfs/aegir/wiki/Github-Actions-Setup

@mpetrunic
Copy link
Member

if it helps
https://github.com/ipfs/aegir/wiki/Github-Actions-Setup

It helped :) I've added dependency caching to make thing a bit faster. Disabled bundle size checker since we still depend on bcrypto and node-forge which brings bundlesize to ~200kb. I think idea was to move crypto we need to libp2p-crypto but afaik it's not done yet

@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@2df5f95). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #89   +/-   ##
=========================================
  Coverage          ?   89.90%           
=========================================
  Files             ?       16           
  Lines             ?     1773           
  Branches          ?      246           
=========================================
  Hits              ?     1594           
  Misses            ?      179           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2df5f95...5d36667. Read the comment docs.

mpetrunic
mpetrunic previously approved these changes Jan 27, 2021
@vasco-santos
Copy link
Collaborator

I think idea was to move crypto we need to libp2p-crypto but afaik it's not done yet

Yes, that is still the plan, but we could not tackle it yet

@hugomrdias
Copy link
Contributor Author

hugomrdias commented Feb 12, 2021

Some metrics with the new aegir

Screenshot 2021-02-12 at 20 26 07

Size is bigger because licences are inlined instead of in a separate file.

@mpetrunic
Copy link
Member

You've added incremental flag to tsconfig? How do you deal with bugs it's causing (not deleting old files etc)?
How come you removed cache?

@hugomrdias
Copy link
Contributor Author

The cache wasn't letting me pull the latest aegir. I will add it back once before merging.
Can you elaborate on the bugs from incremental?

@hugomrdias
Copy link
Contributor Author

Actually theres no real need for incremental right now, gonna remove it.

@hugomrdias
Copy link
Contributor Author

@mpetrunic can you point me to some issues created by --incremental ?

@mpetrunic
Copy link
Member

@mpetrunic can you point me to some issues created by --incremental ?

Most annoying it that it doesn't remove deleted source file from dist/build directory. That said, you are deleting whole dist directory on every build so it doesn't really matter but that also means that we are deleting tsconfig.tsbuildinfo so incremental flagisn't doing anything afaik.

@hugomrdias
Copy link
Contributor Author

Screenshot 2021-02-18 at 16 49 44

docs preview

@hugomrdias hugomrdias mentioned this pull request Feb 18, 2021
3 tasks
@mpetrunic
Copy link
Member

@hugomrdias aegir is dropping node12 support? We need to remove it from CI here as well?

@hugomrdias
Copy link
Contributor Author

@hugomrdias aegir is dropping node12 support? We need to remove it from CI here as well?

aegir still works in node12 because the github-action needs it but everything else yes https://github.com/ipfs/community/blob/master/CONTRIBUTING_JS.md#supported-versions

The IPFS JavaScript projects work with the Current and Active LTS versions of Node.js and respective npm version that gets installed with Node.js. Please consult nodejs.org for LTS timeline.

@hugomrdias
Copy link
Contributor Author

@mpetrunic its finally ready

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

awesome tnx @hugomrdias

@mpetrunic mpetrunic merged commit 81a70d1 into ChainSafe:master Feb 24, 2021
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.

4 participants