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 Linux installation problem, move fsevents to optional dependencies #22

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

shreyansh23
Copy link
Contributor

@shreyansh23 shreyansh23 commented Mar 24, 2019

Description: What does this PR achieve? [feature|hotfix|refactor]

Fix issue issue in Linux installation + Migrate to Babel 7 🎉
Related to PR #19
Closes issue #16

  • Earlier there were problems in Linux installation. The problem was due to fsevents package.
  • The babel version 6 was used. But the testing required version 7. So, the tests were failing! (Ref).

Solution

  • fsevents is moved to optionalDependencies which is where it should be present.
  • Migrate Babel from version 6 to 7

Implementation
Implemenation was done following the steps suggested by https://babeljs.io/docs/en/configuration

Technical: What should be noted about the implementation?

n/a

Testing: Steps for reviewer to reproduce / verify this PR fixes the problem?

Can be tested by pulling the branch. Then follow the steps suggested in the README.md
Steps:

yarn run lerna bootstrap
yarn run lerna link
cd packages/ia-components
yarn run storybook

Evidence: If this PR touches UI, please post evidence (screenshot) of it behaving correctly:

Screenshot from 2019-03-24 18-09-46

Screenshot from 2019-03-24 19-28-05

Screenshot from 2019-03-24 18-11-13

@shreyansh23
Copy link
Contributor Author

@bfalling @iisa @mekarpeles
Please review the PR

@mitra42
Copy link
Collaborator

mitra42 commented Mar 24, 2019

Careful with Babel - it seems that none of us really understand it, and there are incompatibilities with what you have to include in what config files (babel.rc and webpack.config) for different versions. I have a complete mess in the IAUX2 branch of dweb-archive and cannot find any combination of babel that will run properly with webpack and also at runtime. On Friday @iisa and I got it compiling with up to date babel, but it complained about a runtime missing - adding the runtime meant it wouldn't compile.

@mitra42
Copy link
Collaborator

mitra42 commented Mar 24, 2019

If you want to try this ....
Check out dweb-mirror#iaux2 and checkout iaux#dwebnew. They should be in side-by-side directories (e.g. git/dweb-mirror and git/iaux ). Then try cd dweb-mirror && webpack --mode development. I cannot get it to webpack except by removing some things, in particular plugin-transform-runtime and it seems to need one of those plugins to run in the browser (Its not clear if its babel-plugin-transform-runtime or @babel/plugin-transform-runtime).

Note - you'll probably need to do cd dweb-mirror && yarn install and also maybe cd dweb-mirror && ./install.sh before webpack-ing.

If you figured out how to get this to work it would be huge.

Copy link
Contributor

@iisa iisa left a comment

Choose a reason for hiding this comment

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

@shreyansh23 thanks for the start off this.a couple of questions.

packages/ia-design-system/webpack.config.js Outdated Show resolved Hide resolved
@@ -21,9 +21,10 @@
"@storybook/addon-knobs": "^4.1.4",
"@storybook/react": "^4.1.4",
"assert": "^1.4.1",
"babel-core": "^7.0.0-bridge.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why are we using babel-core instead of @babel/core v. 7?

Copy link
Contributor

Choose a reason for hiding this comment

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

we already use "@babel/core": "^7.2.2", at the first line, we should not need the two unless it is for a certain reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iisa
Your concern is valid. I will need to check with docs and babel-upgrade package which I used for upgrading the version. I will do some research and reply.

Copy link
Contributor Author

@shreyansh23 shreyansh23 Mar 24, 2019

Choose a reason for hiding this comment

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

@iisa I think this happened because ia-components/package.json was already using Babel 7 and babel-upgrade package which I used was designed to migrate from Babel 6 to Babel 7. So changes in this package.json were not required. Thanks for reviewing.
I will update the PR.

@iisa I just checked the the babel-upgrade package's documentation. Please have a look. Ref: https://github.com/babel/babel-upgrade

Relevant Screenshot from the page:
Screenshot from 2019-03-24 23-56-00

Hence the behavior was intendend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jest JS docs say otherwise. Perhaps upgrade tool is behind. Pleaes double check. https://jestjs.io/docs/en/getting-started

Screen Shot 2019-03-25 at 9 18 15 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iisa I again cross-checked.
It is strange but babel-core is needed in case of jestJS.
Relevant links:
1: jestjs/jest#6913 (comment)
2: jestjs/jest#6662 (comment)
All the the solutions say the same thing, i.e. keep both @babel/core and babel-core in case of jest as a dependency.
So I strongly think that the babel-upgrade is doing the right thing.
Also if you see link 1, removing babel-core gives errors.
So concluding on the note that babel-upgrade is doing the right thing in my opinion.

@shreyansh23
Copy link
Contributor Author

If you want to try this ....
Check out dweb-mirror#iaux2 and checkout iaux#dwebnew. They should be in side-by-side directories (e.g. git/dweb-mirror and git/iaux ). Then try cd dweb-mirror && webpack --mode development. I cannot get it to webpack except by removing some things, in particular plugin-transform-runtime and it seems to need one of those plugins to run in the browser (Its not clear if its babel-plugin-transform-runtime or @babel/plugin-transform-runtime).

Note - you'll probably need to do cd dweb-mirror && yarn install and also maybe cd dweb-mirror && ./install.sh before webpack-ing.

If you figured out how to get this to work it would be huge.

@mitra42
I will definitely try that.

@traceypooh
Copy link
Contributor

@mitra42 well, technically speaking i understand babel very well.
i set us up with it ~4y+ ago;
then moved us from a "push out" script to package.json script;
then helped move it to webpack

the problem has been more in last ~2y we haven't been as rigorous about testing updates -- IE esp. got a bit tricky some additional polyfill requirements, etc.

@shreyansh23
Copy link
Contributor Author

@traceypooh Would you also please review the PR. I think we should leave no stone unturned.

@mitra42
Copy link
Collaborator

mitra42 commented Mar 24, 2019

Glad to hear that @traceypooh, we didn't think to ask you.

Even on stackexchange people dont seem to fully understand what versions of what packages are needed for current versions, and the problem seems to be that babel does a crappy job of defining dependencies so if you don't have just the right set it gives incomprehensible error messages.

Copy link
Contributor

@iisa iisa left a comment

Choose a reason for hiding this comment

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

more comments @shreyansh23

@@ -21,9 +21,10 @@
"@storybook/addon-knobs": "^4.1.4",
"@storybook/react": "^4.1.4",
"assert": "^1.4.1",
"babel-core": "^7.0.0-bridge.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Jest JS docs say otherwise. Perhaps upgrade tool is behind. Pleaes double check. https://jestjs.io/docs/en/getting-started

Screen Shot 2019-03-25 at 9 18 15 AM

@@ -2077,6 +2077,11 @@ babel-core@^6.0.0, babel-core@^6.26.0:
slash "^1.0.0"
source-map "^0.5.7"

babel-core@^7.0.0-bridge.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

please note to update yarn.lock once this is resolved

@@ -4,19 +4,20 @@
"license": "AGPL-3.0-only",
"private": true,
"dependencies": {
"babel-preset-react": "^6.24.1",
"@babel/core": "^7.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@babel/core has been added twice, here and in devDependencies.

@iisa
Copy link
Contributor

iisa commented Mar 29, 2019

@shreyansh23 hi, can you pull the FSEvents to optional dependency as its own PR?

@shreyansh23 shreyansh23 force-pushed the hotfix/linux-fsevents branch from dd6115b to b2d9f80 Compare March 29, 2019 18:16
@shreyansh23 shreyansh23 changed the title Fix Linux installation problem + Migrate to Babel 7 Fix Linux installation problem, move fsevents to optional dependencies Mar 29, 2019
@shreyansh23
Copy link
Contributor Author

Updating this PR.
This PR only fixed move fsevents to optional dependencies.
Babel Migration is handled by PR #51

Copy link
Contributor

@iisa iisa left a comment

Choose a reason for hiding this comment

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

LGTM

@iisa
Copy link
Contributor

iisa commented Mar 29, 2019

Babel upgrade has been moved to a different PR #51

@iisa iisa merged commit 80dee08 into internetarchive:master Mar 29, 2019
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