Use ES6 modules, use rollup for module bundling #198

simon04 · 2017-02-04T18:45:51Z

modularize the code using ES6 modules
use rollup for bundling (replaces browserify)
separate holiday file per country in holidays/
run make opening_hours.js to get the bundled file w/o embedded dependencies

ypid · 2017-02-05T21:46:57Z

Thanks very much for the PR! I guess I should do a release before I review and merge this 😉

pke · 2017-02-13T13:39:34Z

Looks interesting. I was planing a code split myself, but using webpack. Basically I think this library should be componentized in a way that the base file just uses the holidays and translations already loaded. Meaning when someone wants to include the script one has to include the required holidays and translations before the core script file. The holiday and language files will register themselves globally under opening_hours.holidays etc.

In SPA based the holidays and languages could then be imported first and opening_hours last:

import "opening_hours/holidays/de"
import oh from "opening_hours/core"

// -- or import all holidays
import "opening_hours/holidays"
import oh from "opening_hours/core"

// -- or import all at once
import oh from "opening_hours"

What u think?

The library is just too huge at the moment with ~350kb. I guess the majority of this is coming from holidays (strings).

ypid · 2017-02-17T23:27:15Z

I finally enabled Travis CI testing for this project and made a release.

Can you try to get the build passing? And don’t worry about the merge conflict.

ypid · 2017-02-18T17:41:43Z

holidays/index.js

@@ -0,0 +1,19 @@
+export {default as at} from './at.js';


I would prefer to have the holidays as simple JSON files for interoperability reasons. Seems like rollup-plugin-json could come in handy for this. Does this make sense and can you try it?

I just found that there is also a YAML plugin ❤️ So I would prefer YAML. But I can also convert that later.

ypid · 2017-02-18T17:52:54Z

rollup.config.js

+    format: 'cjs',
+    plugins: dependencies ? [
+        nodeResolve(),
+        common()


Can you end all list items with a comma please? This makes moving items around really easy and I hate that JSON does not allow this.

@ypid so can be enforced on ci by running eslint with the associated eslintrc & dev requirement packages

ypid

Thanks very much. Looks good. Can you check the review comments?

Do you have an idea how to deal with the different formats now. The opening_hours.js is now only cjs, before it was also amd and iife at the same time. I guess I will just build those files on release and include them in the npm package?

ypid · 2017-02-18T20:17:07Z

rollup.config.js

+export default {
+    banner: banner,
+    entry: './index',
+    format: 'cjs',


I guess you could also add: moduleName: 'opening_hours', so that --format iife works directly.

simon04 · 2017-02-20T21:13:20Z

@ypid, thank you for the review. I hopefully addressed all comments plus performed the (cumbersome) extra tasks converting the holiday definitions to YAML.

@pke, let's address this in a separate issue/PR in order to not end up with one monster PR which is impossible to review.

ypid · 2017-02-20T21:24:39Z

@simon04 Thanks so much for you work, also with updating the holiday definitions! I have been working on a Python script to update YAML format over the last few days to solve #192 so this is coming together nicely.

But the older NodeJS versions, I guess https://babeljs.io/ can be used for that? Or maybe drop the const in rollup.config.js?

ypid · 2017-02-20T21:33:31Z

Could you make a changelog entry now that you rebased on latest master? 😉

ypid · 2017-02-20T21:37:03Z

You can drop support for legacy versions if you want to. According to the LTS schedule they are EOL anyway. I just supported them because interestingly when I tried with the latest NodeJS release, basically nothing broke.

simon04 · 2017-02-20T22:01:59Z

Node.js: 0.12 compatibility is back. Had to amend my commit for CHANGELOG a few times to get the syntax for links right.

simon04 · 2017-02-20T22:23:36Z

Jay, all tests pass 😄

ypid · 2017-02-20T22:56:40Z

Awesome! I will be working on getting this merged the next days.

Used json2yaml plus manual copying of comments

`amd`, `cjs` and `iife` all in one

Used json2yaml plus manual copying of comments

simon04 · 2017-02-21T20:04:40Z

I rebased again and introduced an initial renaming commit 378b226 to retain the Git history for index.js. Additionally, I converted the locales/ strings to YAML format in 880b032.

shouze

LGTM

shouze · 2017-02-24T12:45:34Z

@ypid let's go for a merge?

ypid · 2017-02-24T13:58:52Z

Sure, working on it.

Ref: opening-hours#198

ypid

@simon04 Thanks very much, awesome work!

pke · 2017-03-11T09:47:47Z

@simon04 now that this is merged, how can we make use of it in es6 to only include what one needs. Namely don't include all the holidays.

Also maybe u have a clue why the peer dependency of momentjs is still installed underneath this modules node folder even if my app already provided a newer momentjs library.

@ypid what for is moment js used here? For date calculation or merely to display dates and times? If the latter then maybe we could split that out in a new bundle.
If moment js is used for parsing dates or times then there are smaller es6 friendly Libs available . I'll open a new issue for that.

ypid · 2017-03-11T10:13:49Z

now that this is merged, how can we make use of it in es6 to only include what one needs.

What you are asking for is already tracked here #143 which should be used for discussion/constructive input (to which I pointed you already). This PR is not the right place.

what for is moment js used here?

The source is your friend. Currently only to get the list of weekdays in the configured language. But it is optional.

Related to: opening-hours/opening_hours.js#198

ypid self-assigned this Feb 5, 2017

ypid added the type: feature Introduction of new functionality. label Feb 5, 2017

This was referenced Feb 5, 2017

Provide holiday data and basic functions as separate (npm) module #71

Open

Don't use for...in at all for old browsers compatibility #199

Closed

simon04 force-pushed the rollup branch from a59df80 to 1e37200 Compare February 16, 2017 23:22

ypid added this to the v3.6.0 milestone Feb 17, 2017

ypid reviewed Feb 18, 2017

View reviewed changes

ypid requested changes Feb 18, 2017

View reviewed changes

simon04 force-pushed the rollup branch 2 times, most recently from 9af005f to 63d36f9 Compare February 20, 2017 21:00

simon04 force-pushed the rollup branch 2 times, most recently from 28111dd to 8847cf1 Compare February 20, 2017 22:00

simon04 added 7 commits February 21, 2017 20:22

Rename opening_hours.js to index.js

378b226

Use ES6 modules, use rollup for module bundling

c181731

Move holidays README

5f82ace

Convert holidays to YAML

0278146

Used json2yaml plus manual copying of comments

Build umd (Universal Module Definition)

517bf77

`amd`, `cjs` and `iife` all in one

Attempt to make the tests run

ef9b764

Add CHANGELOG entry

d4e4721

Attempt to restore Node 0.12 compatibility

aa4596d

simon04 force-pushed the rollup branch from 8847cf1 to aa4596d Compare February 21, 2017 19:26

Convert locales to YAML

880b032

Used json2yaml plus manual copying of comments

This was referenced Feb 22, 2017

split formatting i18n and i18n from main package #150

Open

Update German SH #192

Closed

shouze approved these changes Feb 22, 2017

View reviewed changes

ypid added a commit to ypid/opening_hours.js that referenced this pull request Feb 26, 2017

Add opening_hours.js to .gitignore which is not tracked anymore

3e9b516

Ref: opening-hours#198

ypid added a commit to ypid/opening_hours.js that referenced this pull request Feb 26, 2017

Make the evaluation tool work again after opening-hours#198

0ab222d

ypid approved these changes Feb 27, 2017

View reviewed changes

ypid merged commit 880b032 into opening-hours:master Feb 27, 2017

ypid added a commit that referenced this pull request Feb 27, 2017

Merge branch '#198'

901730f

ypid added a commit that referenced this pull request Mar 5, 2017

Fix source file reference after switch to rollup. Fixes #198.

76a242b

ypid added a commit to opening-hours/opening_hours_map that referenced this pull request Mar 19, 2017

Update to opening_hours.js build with rollup

ebd0245

Related to: opening-hours/opening_hours.js#198

ypid mentioned this pull request Mar 19, 2017

Rework and using opening_hours build with rollup opening-hours/opening_hours_map#44

Merged

ypid mentioned this pull request Oct 28, 2017

momentjs import makes it no longer optional #243

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ES6 modules, use rollup for module bundling #198

Use ES6 modules, use rollup for module bundling #198

simon04 commented Feb 4, 2017

ypid commented Feb 5, 2017

pke commented Feb 13, 2017 •

edited

Loading

ypid commented Feb 17, 2017

ypid Feb 18, 2017

ypid Feb 18, 2017

ypid Feb 18, 2017

shouze Feb 18, 2017

ypid left a comment

ypid Feb 18, 2017

simon04 commented Feb 20, 2017

ypid commented Feb 20, 2017 •

edited

Loading

ypid commented Feb 20, 2017

ypid commented Feb 20, 2017

simon04 commented Feb 20, 2017

simon04 commented Feb 20, 2017

ypid commented Feb 20, 2017

simon04 commented Feb 21, 2017 •

edited

Loading

shouze left a comment

shouze commented Feb 24, 2017

ypid commented Feb 24, 2017

ypid left a comment

pke commented Mar 11, 2017

ypid commented Mar 11, 2017 •

edited

Loading

Use ES6 modules, use rollup for module bundling #198

Use ES6 modules, use rollup for module bundling #198

Conversation

simon04 commented Feb 4, 2017

ypid commented Feb 5, 2017

pke commented Feb 13, 2017 • edited Loading

ypid commented Feb 17, 2017

ypid Feb 18, 2017

Choose a reason for hiding this comment

ypid Feb 18, 2017

Choose a reason for hiding this comment

ypid Feb 18, 2017

Choose a reason for hiding this comment

shouze Feb 18, 2017

Choose a reason for hiding this comment

ypid left a comment

Choose a reason for hiding this comment

ypid Feb 18, 2017

Choose a reason for hiding this comment

simon04 commented Feb 20, 2017

ypid commented Feb 20, 2017 • edited Loading

ypid commented Feb 20, 2017

ypid commented Feb 20, 2017

simon04 commented Feb 20, 2017

simon04 commented Feb 20, 2017

ypid commented Feb 20, 2017

simon04 commented Feb 21, 2017 • edited Loading

shouze left a comment

Choose a reason for hiding this comment

shouze commented Feb 24, 2017

ypid commented Feb 24, 2017

ypid left a comment

Choose a reason for hiding this comment

pke commented Mar 11, 2017

ypid commented Mar 11, 2017 • edited Loading

pke commented Feb 13, 2017 •

edited

Loading

ypid commented Feb 20, 2017 •

edited

Loading

simon04 commented Feb 21, 2017 •

edited

Loading

ypid commented Mar 11, 2017 •

edited

Loading