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

Upgrade Babel 7 #332

Merged
merged 16 commits into from Jan 15, 2019
Merged

Upgrade Babel 7 #332

merged 16 commits into from Jan 15, 2019

Conversation

philihp
Copy link
Contributor

@philihp philihp commented Jan 6, 2019

Yak shave to end all yak shaves

Tests run

  • npm run test
  • npm run prepare
  • npm run docsify
  • npm run storybook
  • npm run dev
  • npm run test:integration
  • ...anything else to try out?

@nicolodavis
Copy link
Member

Awesome!

Can you take a look at the integration test (npm run test:integration)? Looks like it's failing (although the script itself is not reporting itself as failed, which I should fix).

Reference: https://semaphoreci.com/nicolodavis/boardgame-io/branches/pull-request-332/builds/1

"babel-cli": "^6.26.0",
"babel-core": "^6.26.3",
"@storybook/react": "^4.1.4",
"ajv": "^6.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

What is this dependency for?

Copy link
Contributor Author

@philihp philihp Jan 7, 2019

Choose a reason for hiding this comment

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

Resolves this warning

npm WARN ajv-keywords@3.2.0 requires a peer of ajv@^6.0.0 but none is installed. You must install peer dependencies yourself.

It's a quick/easy fix to a known problem described in webpack/schema-utils#23 (comment) and npm/npm#19877 and npm/npm#19877 (comment)

@philihp
Copy link
Contributor Author

philihp commented Jan 7, 2019

Holding tight, npm run dev needs bumping too. Looks like babel-watch just got a version that supports 7, too.

@philihp
Copy link
Contributor Author

philihp commented Jan 11, 2019

Nevermind... babel-watch still doesn't support Babel 7 yet. I think I can accomplish the same thing though with nodemon or @babel/cli (which is already included)... but they don't play well when they have to compile things outside of their dir (e.g. module-resolver going to ../../packages). @nicolodavis any feelings/aversions toward creating an examples/react/package.json and putting the examples dependencies in that, similar to how examples/react-native has its own package.json?

Pros:

  • Removes A LOT of devDependencies from the root project (e.g. chess.js)
  • Removes need for babel-watch entirely.
  • module-resolver here now goes to ../../dist/ which is packaged with Babel and Rollup and what is ultimately distributed, instead of ../../package/ which is just built on-the-fly with Babel.

Cons:

  • When working on an example project, you don't have the convenience of being able to make a change to core/client. Maybe this is a good thing though? You can still make changes to the boardgame.io library and validate them with unit tests using npm run test:watch, perhaps not with an example app.

@vdfdev
Copy link
Contributor

vdfdev commented Jan 13, 2019

I am just afraid that not being able to validate with the example app will make our life harder to manually test features. And this is important specially for some browser-dependent features like drag and drop that I am developing now.

@philihp
Copy link
Contributor Author

philihp commented Jan 13, 2019

Could the drag-and-drop components be developed within /examples/react/, and then moved into /src/ui/ when they're more solidified?

@vdfdev
Copy link
Contributor

vdfdev commented Jan 13, 2019

Not really, because I am changing Token.js which is already included in /src/ui/... I could make a full new copy of the file and use that instead for development... That's a way.

I think the drag and drop should be mostly done now, but I just wonder that this might also happen for other features that are not very easily unit-testable.

@vdfdev
Copy link
Contributor

vdfdev commented Jan 13, 2019

But at the same time, I love TDD and I think that forcing people to look more into the tests and less into manually testing is a good thing. So I don't know, im just pointing the trade-offs here :)

@philihp
Copy link
Contributor Author

philihp commented Jan 13, 2019

Yep yep, it is a tradeoff for sure.

Quick question... are you using npm run storybook for developing the components?

@nicolodavis
Copy link
Member

I use the examples a lot when developing to do quick sanity tests. I think it should still be possible to have a separate package.json with all the dev dependencies and also "symlink" the boardgame.io dependency to the code in the repo so that you can make changes and see them live, no?

Also, I don't think anyone uses storybook. I'm in favor of removing it altogether.

@nicolodavis
Copy link
Member

Any thoughts about the Babel regenerator runtime error that you get when running the integration test?

@nicolodavis
Copy link
Member

Removed storybook in f853da8.

@philihp
Copy link
Contributor Author

philihp commented Jan 15, 2019

Like symlinking the package into node_modules? Babel 7 no longer transpiles files in there by default... might take some more configuration.

babel/babel#8711 (comment)

https://babeljs.io/docs/en/v7-migration#config-lookup-changes

@nicolodavis
Copy link
Member

nicolodavis commented Jan 15, 2019

I see. OK, then let's just keep the examples in the same package.json as the main project until we can figure out a way to split them and still retain the ability to see library changes in the examples automatically.

"dev": true,
"requires": {
"core-js": "2.5.5",
"promise-polyfill": "7.1.2",
"whatwg-fetch": "2.0.4"
},
"dependencies": {
"chokidar": {
Copy link
Contributor Author

@philihp philihp Jan 15, 2019

Choose a reason for hiding this comment

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

this was weird.. it had the same hash as whatwg-fetch, too... responsible for the last 5 build fails.

would be great to see what @firebase/polyfill looked like for 0.3.3 and why this happened, but monorepos 🤷‍♂️

@philihp
Copy link
Contributor Author

philihp commented Jan 15, 2019

@nicolodavis integration test works now, that's a clever test... I like it. rollup.npm.js needed an option added.

@philihp
Copy link
Contributor Author

philihp commented Jan 15, 2019

I guess last remaining unsolved question is how to get the examples webapp to hot-reload changes to a package outside of its dir.

@nicolodavis
Copy link
Member

Thanks @philihp!

Let's punt that to another PR. This is a good change to get in right now.

Made one change, which is to remove the async-to-generator altogether (which avoids all the regeneratorRuntime hassle and also results in a smaller bundle).

replace({
'process.env.NODE_ENV': JSON.stringify('production'),
}),
uglify(
Copy link
Member

Choose a reason for hiding this comment

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

Missed this. Why are we no longer minifying the code here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing you deleted this because it was generating some errors and forgot to add it back.
Added rollup-plugin-terser instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I removed it to simplify things. At first it was having some errors so I upgraded it but then the config for it was broken. rollup-plugin-terser looks a better solution anyway, and zero config 💯

@nicolodavis nicolodavis merged commit 1f71bbd into boardgameio:master Jan 15, 2019
@nicolodavis
Copy link
Member

Also, about npm run dev, I think we could use nodemon / babel-node as shown here: https://hackernoon.com/using-babel-7-with-node-7e401bc28b04

@philihp
Copy link
Contributor Author

philihp commented Jan 15, 2019

Thanks for the merge, the scope of this was starting to make me go crazy 🤕 . I'll take a stab at nodemon/babel-node. There's GOTTA to be a way to make this work.

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.

None yet

3 participants