feat: add flow type annotations #49

vmx · 2018-04-07T20:16:23Z

Also configure build/test/lint scripts for Flow typed JS

Run flow check after linting
Use AEgir config to use babel-loader for webpack builds
Use different babel config for browser builds to avoid inlining source maps

This PR supersedes #47. I didn't want to hijack @Gozala's PR. This PR is just a cleaned up version of his work. All credits go to him.

@Gozala please review if it is still doing what it's supposed to do. I've changed the npm scripts a bit to run the build before the test runs. I also removed the hack around Mocha, it's now part of AEgir in the flow branch.

garrensmith

This looks really good. Would it be possible to move the .babelrc and aegir.js webpack settings into aegir itself? That would really reduce the boilerplate code for other projects.

garrensmith · 2018-04-08T07:39:31Z

.aegir.js

+  // Have not used webpack so just copied proposed setup from
+  // https://babeljs.io/docs/setup/#installation
+  webpack: {
+    module: {


I think ideally we want to merge this into the aegir flow branch so that we can get inline source maps for tests.

With and without the file I don't get the correct line numbers. What about if I remove this completely and having proper line numbers in stack traces of the browser tests will be an open issue?

Update: Nevermind, it had to do with some Babel config. I got it working.

garrensmith · 2018-04-08T07:40:10Z

example.js

@@ -1,5 +1,3 @@
-'use strict'
-
 const multihash = require('multihashes')


I think we might still need use strict here since this isn't a flow file.

fahrradflucht · 2018-04-08T10:57:38Z

src/index.js

-exports.defaultLengths = cs.defaultLengths
-
-const varint = require('varint')
+import bs58 from 'bs58'


While now working myself on typing multibase I came to ask: Don't you have to include type definitions for this? Or are fine with having all our externals default to any?

While now working myself on typing multibase I came to ask: Don't you have to include type definitions for this? Or are fine with having all our externals default to any?

I think in case of bs58 / varint it might make sense to do it with something like flow-typed as API surface is tiny, but in general trying to cover all the dependencies can be a rabbit hole and in my experience it's more practical to do that afterwards.

Gozala

Thanks @vmx for pushing this further. Everything looks good to me.

Gozala · 2018-04-10T10:24:22Z

package.json

    "buffer-equal": "1.0.0",
    "chai": "^4.1.2",
    "dirty-chai": "^2.0.1",
+    "flow-bin": "^0.66.0",


I would recommend upgrading to [email protected] at this point and probably it would probably make more sense to have flow-bin bundled with aegir.

vmx · 2018-04-10T16:06:27Z

I've moved the webpack specific things from .aegir.js and the .babelrc into AEgir.

Though there is an issue now. babel --watch doesn't work anymore as there is no .babelrc file. I have no clue how to fix this. I didn't find a way to make the Babel CLI pick up a .babelrc that is bundled with AEgir. @Gozala Do you have an idea?

Gozala · 2018-04-11T06:40:30Z

I've moved the webpack specific things from .aegir.js and the .babelrc into AEgir.

Though there is an issue now. babel --watch doesn't work anymore as there is no .babelrc file. I have no clue how to fix this. I didn't find a way to make the Babel CLI pick up a .babelrc that is bundled with AEgir. @Gozala Do you have an idea?

Sorry I'm not that JS tooling savvy to be honest. Looking at the babel documentation it does not seems like you could provide alternative path to .babelrc.

Given this limitation I think best option would be to add some command to AEgir itself to do just what npm start was configured to do here. In fact it may be a better option regardless that way npm start script colud be just start: "aegir --watch build" or something.

vmx · 2018-04-11T09:34:01Z

@Gozala The plan surely is to have a an aegir watch, though I have no clue how to do that (yet). But I'm sure I'll find a way/someone who knows.

Gozala · 2018-04-11T11:43:36Z

@Gozala The plan surely is to have a an aegir watch, though I have no clue how to do that (yet). But I'm sure I'll find a way/someone who knows.

I must have misunderstood you. I don't really know how AEgir is set up sadly. I also realize now that it probably doesn't even makes use of babel so it's not just matter of exposing --watch as initially thought.

I took another look and it seems that babel-cli has extends option that takes "A path to an .babelrc file to extend" which sounds promising, as you could have a default .babelrc bundled with AEgir and presumably watch command would just invoke babel-cli passing it extends option pointing to that bundled configuration.

vmx · 2018-04-11T13:43:17Z

I also couldn't get extends working. I added up implementing a watcher myself (ipfs/aegir@8fae4f4). That one can also be used to replace flow-copy-source. It's not ideal (I dislike re-inventing the wheel), but seems to do the job.

For the record: What kind of worked with babel-cli was npx babel --no-babelrc --presets env,flow-node --watch. Though I didn't see how I could possible pass options in. I guess you'd need to create a custom preset.

vmx · 2018-04-11T13:46:29Z

I think the only thing left is to move the flow stuff (the type checking command, files copying when watching and the .flowconfig) into AEgir. I won't have time this week, so if anyone wants to pick it up, that would be great.

garrensmith · 2018-04-11T14:16:25Z

I would be keen to take a stab at that. But only next week

…

On Apr 11, 2018, at 3:46 PM, Volker Mische ***@***.***> wrote: I think the only thing left is to move the flow stuff (the type checking command, files copying when watching and the .flowconfig) into AEgir. I won't have time this week, so if anyone wants to pick it up, that would be great. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

vmx · 2018-04-11T14:45:22Z

For everyone watching this PR: If shouldn't feel blocked on the changes on AEgir if you want to port other modules to Flow. You can use this PR as a template, but don't need to stay up-to-date. We'll figure things out later.

victorb · 2018-05-02T13:05:40Z

package.json

@@ -2,19 +2,23 @@
  "name": "multihashes",
  "version": "0.4.13",
  "description": "multihash implementation",
-  "main": "src/index.js",
+  "main": "lib/index.js",


So this will now point at files that doesn't exists when we checkout the code right? Only after running build? How do we make this easy to work with when you have multiple modules that needs building and npm-linking?

I think you will run a watcher (part of AEgir) in the project that you link. Is that correct @Gozala?

So in practice, you would run one watcher for builds and one watcher for tests, per each project you link + top project?

I'm pretty sure this is alright, but let's check in with the person who probably links most projects currently, @diasdavid. Does the above flow work for you?

I think you will run a watcher (part of AEgir) in the project that you link. Is that correct @Gozala?

That is at least in my understand on what we agreed to try, except you don't necessary need to run watcher on linked packages that you're not making changes to.

daviddias · 2018-05-03T11:22:24Z

Unfortunately, I do not have the capacity to page in into all the discussions and act as the cat herder for this one. @vmx took the lead to move this endeavor forward -- ipfs/js-ipfs#1260. I did share my thoughts and past experiences with this project on tooling and build steps here -- https://github.com/ipfs/community/issues/278.

I'm pinging @hugomrdias here who has shared with me a lot of very careful thoughts on making this very smooth for devs and consumers of these modules. AFAIU, webpack 4 is a game changer, folks do not use anymore the browser key on package.json to do browserify like shims, instead the keys browser, module, main are checked by modern bundlers (webpack 4, rollup and parcel) to pick the right source to use.

Also, I worry that things like the notes here -- #49 (comment) -- become tribal knowledge. Some of the litmus test to move the awesome endeavor flow are:

Can a user, independent to the IPFS project, use one of our modules without buying in into our tooling? If a user can't use multihash without aegir, then something went very wrong.
Is the dev flow easy to understand and there is documentation to ramp up folks?

Both these questions should have a yes answer.

vmx · 2018-05-04T00:03:51Z

All changes I wanted to be part of AEgir are there now. Please review ipfs/aegir#213 as that's the branch that will be used if we merge this PR.

Also configure build/test/lint scripts for Flow typed JS - Run flow check after linting - Use AEgir config to use babel-loader for webpack builds - Use different babel config for browser builds to avoid inlining source maps

Without setting the `BABEL_ENV` to `umd` failures in the Browser tests won't show the correct files/line numbers in the trace.

Transpiling with Babel is now part of AEgir.

For transpiling with Babel no project specific `.babaelrc` file is needed.

The Flow type checking is now part of `aegir lint`.

`flow-copy-source` is no longer needed as its functionality (copying files from `src` to `lib` and adding the `flow` extension) is now part of `aegir build`.

vmx · 2020-06-16T12:26:14Z

We are using TypeScript types in JSDoc instead. That endevour is tracked at ipfs/js-ipfs#2945. Hence closing this issue. Thanks everyone who were involved in trying to make Flow happen.

This was referenced Apr 7, 2018

Add flow type annotations. #47

Closed

Proposal to move to move to Flow types ipfs/js-ipfs#1260

Closed

garrensmith reviewed Apr 8, 2018

View reviewed changes

fahrradflucht reviewed Apr 8, 2018

View reviewed changes

vmx force-pushed the vmx-flow branch from 774a36b to 7800e62 Compare April 9, 2018 09:06

Gozala reviewed Apr 10, 2018

View reviewed changes

Gozala mentioned this pull request Apr 10, 2018

More type friendly API #50

Closed

vmx mentioned this pull request May 2, 2018

Add flow type annotations. multiformats/js-multiaddr#51

Closed

victorb reviewed May 2, 2018

View reviewed changes

vmx force-pushed the vmx-flow branch from ba86cc9 to a56f7e7 Compare May 3, 2018 23:55

vmx force-pushed the vmx-flow branch from a56f7e7 to e160286 Compare May 4, 2018 00:07

Gozala and others added 8 commits May 4, 2018 02:09

feat: add flow type annotations

95919eb

Also configure build/test/lint scripts for Flow typed JS - Run flow check after linting - Use AEgir config to use babel-loader for webpack builds - Use different babel config for browser builds to avoid inlining source maps

fix: set Babel env for Browser tests correctly

fad6be8

Without setting the `BABEL_ENV` to `umd` failures in the Browser tests won't show the correct files/line numbers in the trace.

fix: move webpack configuration to AEgir

ba01897

Transpiling with Babel is now part of AEgir.

chore: update flow-bin package

7da7a40

fix: Babel transpiling is now part of AEgir

440c19f

For transpiling with Babel no project specific `.babaelrc` file is needed.

fix: watching files is now part of AEgir

ec9b091

fix: type checking is now part of AEgir

bbd82e2

The Flow type checking is now part of `aegir lint`.

fix: copying Flow files is now part of AEgir

bacfb5b

`flow-copy-source` is no longer needed as its functionality (copying files from `src` to `lib` and adding the `flow` extension) is now part of `aegir build`.

vmx force-pushed the vmx-flow branch from e160286 to bacfb5b Compare May 4, 2018 00:10

daviddias added the status/ready Ready to be worked label Jun 19, 2018

vmx closed this Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flow type annotations #49

feat: add flow type annotations #49

vmx commented Apr 7, 2018

garrensmith left a comment

garrensmith Apr 8, 2018

vmx Apr 9, 2018 •

edited

Loading

garrensmith Apr 8, 2018

fahrradflucht Apr 8, 2018

Gozala Apr 10, 2018 •

edited

Loading

Gozala left a comment

Gozala Apr 10, 2018

vmx commented Apr 10, 2018

Gozala commented Apr 11, 2018

vmx commented Apr 11, 2018

Gozala commented Apr 11, 2018

vmx commented Apr 11, 2018

vmx commented Apr 11, 2018

garrensmith commented Apr 11, 2018 via email

vmx commented Apr 11, 2018

victorb May 2, 2018

vmx May 2, 2018

victorb May 2, 2018

Gozala May 2, 2018 •

edited

Loading

daviddias commented May 3, 2018

vmx commented May 4, 2018

vmx commented Jun 16, 2020

		@@ -1,5 +1,3 @@
		'use strict'

		const multihash = require('multihashes')

feat: add flow type annotations #49

feat: add flow type annotations #49

Conversation

vmx commented Apr 7, 2018

garrensmith left a comment

Choose a reason for hiding this comment

garrensmith Apr 8, 2018

Choose a reason for hiding this comment

vmx Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

garrensmith Apr 8, 2018

Choose a reason for hiding this comment

fahrradflucht Apr 8, 2018

Choose a reason for hiding this comment

Gozala Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Gozala Apr 10, 2018

Choose a reason for hiding this comment

vmx commented Apr 10, 2018

Gozala commented Apr 11, 2018

vmx commented Apr 11, 2018

Gozala commented Apr 11, 2018

vmx commented Apr 11, 2018

vmx commented Apr 11, 2018

garrensmith commented Apr 11, 2018 via email

vmx commented Apr 11, 2018

victorb May 2, 2018

Choose a reason for hiding this comment

vmx May 2, 2018

Choose a reason for hiding this comment

victorb May 2, 2018

Choose a reason for hiding this comment

Gozala May 2, 2018 • edited Loading

Choose a reason for hiding this comment

daviddias commented May 3, 2018

vmx commented May 4, 2018

vmx commented Jun 16, 2020

vmx Apr 9, 2018 •

edited

Loading

Gozala Apr 10, 2018 •

edited

Loading

Gozala May 2, 2018 •

edited

Loading