Skip to content
This repository was archived by the owner on Dec 21, 2021. It is now read-only.

Conversation

@aapzu
Copy link
Contributor

@aapzu aapzu commented Jun 1, 2018

No description provided.

@aapzu aapzu requested review from hpihkala and timoxley June 1, 2018 09:34
Copy link
Contributor

@hpihkala hpihkala left a comment

Choose a reason for hiding this comment

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

Needs some manual testing, especially from external projects that use this via npm (in node or webpack). I'll try it out and report back.

return fetch('http://localhost:8081')
.catch((e) => {
if (e.errno === 'ENOTFOUND') {
throw new Error('Integration testing requires the api and http-api ("entire stack") to be run in the background. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Use correct project names: "api and http-api" => "engine-and-editor and data-api"
  • Could also check that http://localhost:8890 (data-api) responds

package.json Outdated
"url": "git://github.com/streamr-dev/streamr-client.git"
},
"main": "src/index.js",
"main": "dist/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Filename is probably not index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not. Good catch!

"scripts": {
"build": "webpack --env dev && webpack --env prod && npm run test",
"dev": "webpack --progress --colors --watch --env dev",
"build": "NODE_ENV=production webpack",
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, npm build enforced running tests. I think it's a pretty justifiable idea. For reference, many other build systems, for example gradle and maven always run tests after building too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously tests were run but not enforced (code was built no matter what the result of the tests were). It was a bug though, and not relevant now.
I personally don't think that failing tests should prevent building, but publishing. I'd rather put the running of tests into eg. pre-commit hook (now when we don't have a CI yet).
If you want to put it back to this command though, we can ofc do it before setting up a proper CI

@aapzu
Copy link
Contributor Author

aapzu commented Jun 1, 2018

@hpihkala pls test again now. Fixed the examples (and tidied them up a bit)

@hpihkala
Copy link
Contributor

hpihkala commented Jun 2, 2018

I noticed that the file size of the .web.min.js had grown from 64kB in v0.10.1 to 143kB in v0.10.2. I managed to track down why. The reason was that the http and https modules were being webpacked, but they are only used in node to set keepalive for the http agent. I added browser shims for them, bringing the file size back down to ~68kB minified.

Also did some minor cleanup. All the examples are working now. LGTM. To be published as v0.11.0.

Copy link
Contributor

@timoxley timoxley left a comment

Choose a reason for hiding this comment

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

lgtm

@aapzu
Copy link
Contributor Author

aapzu commented Jun 4, 2018

@hpihkala I'll make a pre-release first (v0.11.0-beta.1) and start to use that in marketplace to be sure that it really works. then later this week we can publish it as a proper version

@aapzu aapzu merged commit 8b28fb4 into master Jun 4, 2018
@aapzu aapzu deleted the CORE-1439-transpile-also-for-node branch June 4, 2018 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants