MadMG / moment-jdateformatparser

Translates the `java.text.SimpleDateFormat` date format to the `moment.js` date format.
MIT License
58 stars 29 forks source link

Loading with Require.js doesn't always work as expected #20

Closed mflorea closed 6 years ago

mflorea commented 7 years ago

I'm using the following code to load moment-jdateformatparser:

require.config({
  paths: {
    'moment': 'path/to/moment',
    'moment-jdateformatparser': 'path/to/moment-jdateformatparser'
  },
  shim: {
    'moment-jdateformatparser': ['moment']
  }
});

require(['moment', 'moment-jdateformatparser'], function(moment) {
  moment().toMomentFormatString('yyyy/MM/dd HH:mm');
});

This fails from time to time complaining that toMomentFormatString is not available. It happens most often on IE11. The issue is that moment-jdateformatparser extends moment using a Require.js callback when require is available (even if moment was previously loaded). Require.js doesn't call the callback immediately but on the next processor cycle (think about setTimeout(0)) even if the required module is already loaded. Thus you have to do this to make it work, which is cumbersome:

require(['moment-jdateformatparser'], function() {
  // moment-jdateformatparser was loaded but it didn't extend moment yet.
  // We need to put our code in a require callback in order to wait for
  // moment-jdateformatparser to extend moment.
  require(['moment'], function(moment) {
    moment().toMomentFormatString('yyyy/MM/dd HH:mm');
  });
});
herom commented 7 years ago

We'll release a new version (v1.1.0 via npm) today - could you please check once it's out if you see the same behaviour? This would be awesome 👍

dpeger commented 6 years ago

I can confirm that with 1.1.0 moment-jdateformatparser this issue is still valid. As already pointed out by @mflorea the problem is the use of the require callback in lines 184-186 of moment-jdateformatparser.js. By using asynchronous require you get a race condition as moment might already be used before hookMoment is executed.

herom commented 6 years ago

Should be fixed by #27, #29 in version 1.2.1.