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

webpack: Migrate translations.js to use ES2015 syntax. #5430

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

digi0ps
Copy link
Collaborator

@digi0ps digi0ps commented Jun 16, 2017

  • Changed all the require statements to import statements.
  • Removed IIFE wrapper

A part of the PR #5377.
@pweaver

@smarx
Copy link

smarx commented Jun 16, 2017

Automated message from Dropbox CLA bot

@digi0ps, it looks like you've already signed the Dropbox CLA. Thanks!

Copy link
Contributor

@pweaver pweaver left a comment

Choose a reason for hiding this comment

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

Thanks @digi0ps. One more change to clear up the way we handle adding globals to the app. It is probably better if long term we expose globals in one place. Also be setting i18next as the default export, it will make it easier to migrate other files which use the global.

import LngDetector from 'i18next-browser-languagedetector';
import Cache from 'i18next-localstorage-cache';

window.i18n = i18next;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be more clear if this gets exported as the default export and exposed using expose-loader in webpack. That way a random file isn't exporting a global variable like this.

@pweaver
Copy link
Contributor

pweaver commented Jun 22, 2017

It was pointed out that webpack doesn't support transpiling anything other then import/export statements. const declarations won't work in some browsers. It may be wise to change those to var or just migrate this module to typescript.

@codecov-io
Copy link

Codecov Report

Merging #5430 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5430   +/-   ##
=======================================
  Coverage   86.64%   86.64%           
=======================================
  Files         432      432           
  Lines       53565    53565           
  Branches     2092     2092           
=======================================
  Hits        46411    46411           
  Misses       7154     7154

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61744a7...02e0ce1. Read the comment docs.

@showell
Copy link
Contributor

showell commented Jun 22, 2017

For future stuff like this, is it possible to do a whitespace-only change first to make it easier to review and be assured that nothing accidentally changed? Merging now.

@showell showell merged commit f228700 into zulip:master Jun 22, 2017
@digi0ps
Copy link
Collaborator Author

digi0ps commented Jun 23, 2017

I don't get you @showell?

@lonerz
Copy link
Member

lonerz commented Jun 23, 2017

@digi0ps what he means is, make the whitespace changes in a separate commit

@digi0ps
Copy link
Collaborator Author

digi0ps commented Jun 23, 2017

Whitespace changes as in? As far as I know whitespace changes are those stray whitespaces that come into the commit.

@lonerz
Copy link
Member

lonerz commented Jun 23, 2017

Not sure, I just took what Steve said literally

@pweaver
Copy link
Contributor

pweaver commented Jun 23, 2017

If you look at the commit since the outer function boundary was removed, the entire commit was unindented. This makes it very hard to diff what part of the function was changed vs just unindented. If you do the unindent in a separate function Steve can review the changes separately from the whitespace removal.

@showell
Copy link
Contributor

showell commented Jun 23, 2017

Exactly. Thanks for the clear explanation, @pweaver.

@digi0ps
Copy link
Collaborator Author

digi0ps commented Jun 24, 2017

Okay will do, thanks @pweaver.

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

7 participants