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

WIP: Flow specific changes #213

Closed
wants to merge 8 commits into from
Closed

WIP: Flow specific changes #213

wants to merge 8 commits into from

Conversation

vmx
Copy link
Member

@vmx vmx commented Apr 8, 2018

This branch is for collecting Flow specific changes. Goal is to make Flow typing work with as little boilerplate as possible.

vmx added 8 commits May 4, 2018 01:47
With this change it's possible to lint Flow based modules.
With this change the stack traces when running the Node.js tests
now display the correct source line of the error and not the line
of the build.
`aegir build` builds a package for Browsers in the `dist` directory.
The distinction between Browser and Node.js isn't that clear anymore.
Hence this command is renamed to `dist`.
All files in the `src` diectory are now transpiled with Babel into
the `lib` directory.
It is no possible to watch for changes in the source directory and
get the files automatically transpiled. Run AEgir as

    aegir build --no-dist --watch
Add `flow check` to check for type errors as additional
linting step.

The `.flowconfig` file in the root of AEgir is needed as else
the linting would fail (although AEgir doesn't define any Flow
types). This is intentional as this branch should only be used
with projects using Flow. There this error message is really
helpful.
In order to make the Flow types usable for the tooling, the
flow files need to reside in the `lib` directory. The files
are now copied when calling `aegir build`. Watching files
is also supported.
@@ -41,10 +44,11 @@
"documentation": "^5.3.3",
"es6-promisify": "^5.0.0",
"eslint": "^4.13.0",
"eslint-config-aegir": "^1.0.1",
"eslint-config-aegir": "git+https://github.com/ipfs/eslint-config-aegir#flow",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to link to a released package instead of GitHub URL

Copy link
Member Author

Choose a reason for hiding this comment

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

For technical or practical/policy reasons? I'd prefer not publishing temporary forks.

Copy link
Member

Choose a reason for hiding this comment

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

For reproducibility and not breaking stuff in the future. If Github goes down (happens strangely often) or the branch changes, it'll break the assumption that if package.json hasn't changed, we should get the same (~) dependencies. This has bitten us in the past, when cleaning up old repositories but forgetting to update some reference and cases where we want to run tests on old commits.

You can use Prelease tags in npm to publish draft-versions of packages: https://docs.npmjs.com/misc/semver#prerelease-tags

Copy link
Member Author

Choose a reason for hiding this comment

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

So I could do a 1.0.1-flow.1 release and it won't do any harm for anyone not specifying exactly this version?

@victorb
Copy link
Member

victorb commented May 22, 2018

  • Needs to be able to run tests by itself (not having to run build). Should take into account the files argument as well
  • Figure out how to not break paths (mismatch between published module and scm)
  • Evaluate if we can get rid of .flowconfig (quick idea: could we write the file before running the command and remove it again once done? Tricking flow into believing there is one during running it)

@vmx
Copy link
Member Author

vmx commented Jun 16, 2020

We are using TypeScript types in JSDocs instead of Flow. This effort is tracked here: ipfs/js-ipfs#2945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants