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

Coffee -> ES6 #40

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Coffee -> ES6 #40

wants to merge 17 commits into from

Conversation

florrain
Copy link
Owner

@florrain florrain commented Apr 6, 2017

Overview

This PR is bundling a bunch of things that I wanted to apply on the repo to make it a little more up-to-date, and hopefully easier to work with:

  • Translation of CoffeeScript files to Javascript ES6
  • Update of the build pipeline to Gulp/Babel
  • Linting with ESLint + AirBnb codestyle
  • A dev task (as default when running gulp) to watch, lint and transpile
  • The source still has no dependency and can be run on Node >= 0.8 but the dev environment will work with Node >= 4 (6 preferred).
  • Upgrade of all dev dependencies.

Publishing

Open to suggestions - I'm planning on publishing a 1.0.0beta version of the package first, before making it official and stable. I'm not too worried but I could have missed something. 🤞

TODO

  • Changelog
  • Bump the version

Review

Tests have been ported to JS as well, but if you could make sure that the source files (especially the main src/index.js) doesn't introduce regression, that would be fantastic.
Any additional tests are very welcome as well.

Testing

  • Clone the repo and checkout this branch. I personally linked the local branch and installed it in another repo to test imports were still working.
git clone https://github.com/florrain/locale.git
cd locale
git checkout repo-upgrade
npm install
npm link

cd ../express-application
npm link locale # install local version of the package
  • Fire up an Express application
  • Register the middleware
  • Define a route to print the locale
  • Send a request with the "Accept-Language" header and check it is working as expected.

Something like this:

import http from 'http';
import express from 'express';
import locale from 'locale';

const app = express();
app.server = http.createServer(app);

app.use(locale(['en', 'de'], 'en'));

app.get('/', function (req, res) {
  res.json({ headers: req.headers, locale: req.locale });
});

app.server.listen(process.env.PORT || config.port);

console.log(`Started on port ${app.server.address().port}`);

@zmagauina-fn
Copy link

@florrain Can I help get these changes merged in?

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.

2 participants