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

Babel upgrade v7 #584

Closed
wants to merge 16 commits into from
Closed

Babel upgrade v7 #584

wants to merge 16 commits into from

Conversation

nileshgulia1
Copy link
Member

@nileshgulia1 nileshgulia1 commented Dec 12, 2018

Addresses: #580

"prettier": "1.14.3",
"promise-file-reader": "1.0.2",
"query-string": "6.1.0",
"raven": "2.6.4",
"raven-js": "3.27.0",
"razzle": "2.4.0",
"razzle": "3.0.0-alpha.0",
Copy link
Member

Choose a reason for hiding this comment

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

@nileshgulia1 we might want to wait for a final version of Razzle here. I guess this upgrade is necessary for Babel 7? Or does Razzle 2.4 work with Babel 7 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tisto We are having babel 7 presets with razzle v3 . But since we're using razzle's presets for babel, for v7 we have to use razzle v3, otherwise we may opt out using razzle's babel configs.

>
Contributer
</th>
/>
Copy link
Member

Choose a reason for hiding this comment

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

@nileshgulia1 it seems there are a few unrelated changes in the PR (tests, CSS, SVG, Icons). Or am I missing something here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tisto Yes there are some updated snapshots, and few svg's for default semantic theme, which added when working on less v3.9.0 and semantic-ui-less upgrades. Although some SVG's and icons from the default semantic theme can be removed.

@coveralls
Copy link

coveralls commented Dec 18, 2018

Pull Request Test Coverage Report for Build 2757

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 2746: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@sneridagh
Copy link
Member

@nileshgulia1 Crap, look at #585 ... I already worked on that this week :(

I thought that you only were taking care of the Babel 7 testing... try to maintain your PRs as atomic as possible, so only one thing is accomplished. Then reverting or backtracking changes is also easier. You can tell by taking a look into your changelog entry... currently doesn't mention the update (which could be huge) and it's mixed with another biggie, which is Babel 7 upgrade.

I even already made a review on the semantic UI and theme upgrades and I spotted several caveats that I had to fix on my PR. I even made a quick review on the whole resulting live site and it's fine, so I would better use my PR and merge it already. @robgietema already reviewed it today. That doesn't mean that you made a great work, in fact, it's great that you already looked into that and know the innards of semantic and how it works. Keep up the good work!

@tisto regarding the upgrade, I am a bit reluctant if it's required to rely on a super recent Razzle alpha version, I won't merge it (yet) :(

@nileshgulia1 might be a way to upgrade without upgrade razzle?

@nileshgulia1
Copy link
Member Author

nileshgulia1 commented Dec 21, 2018

try to maintain your PRs as atomic as possible, so only one thing is accomplished.

@sneridagh Yes, that's a huge mess, I will take care about that from now on.

might be a way to upgrade without upgrade razzle

I guess we have to refrain from using razzle-babel-presets and manually include the presets : https://github.com/jaredpalmer/razzle/blob/master/packages/babel-preset-razzle/index.js

@nileshgulia1
Copy link
Member Author

@sneridagh would close this ticket?

@sneridagh
Copy link
Member

@nileshgulia1 no, not yet. Let's wait for Timo to weigh in. Please, remove/revert all the things related with the less and semantic upgrade as well (package.json, default theme etc)

@tisto
Copy link
Member

tisto commented Dec 23, 2018

If we go with styleguidist instead of docz I don't need the babel 7 upgrade urgently. Would be nice to have though. Maybe we should keep this PR open until we merged all the stuff that it contains.

@nileshgulia1
Copy link
Member Author

The builds will pass after we get latest less and semantic-ui updates through #585 .

@sneridagh
Copy link
Member

Superseeded by #711

@sneridagh sneridagh closed this Apr 18, 2019
@sneridagh
Copy link
Member

@nileshgulia1 thanks a lot!!, it saved me a lot of time, just pushed #711

@sneridagh sneridagh deleted the babel-upgrade branch April 18, 2019 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants