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

Impossible to use any other date library #333

Closed
ErikPelli opened this issue Aug 28, 2023 · 16 comments · Fixed by #335
Closed

Impossible to use any other date library #333

ErikPelli opened this issue Aug 28, 2023 · 16 comments · Fixed by #335

Comments

@ErikPelli
Copy link

Recent update to ember-power-calendar version 0.20.0 merged the pull request #329 that in fact prevent the usage of any library different from ember-power-calendar-moment and ember-power-calendar-luxon.

Code extracted from the new addon/utils.js file:

const DateLibrary = (() => {
  if (macroCondition(dependencySatisfies('ember-power-calendar-moment', '*'))) {
    return importSync('ember-power-calendar-moment');
  } else if (
    macroCondition(dependencySatisfies('ember-power-calendar-luxon', '*'))
  ) {
    return importSync('ember-power-calendar-luxon');
  } else {
    throw new Error(
      `You have installed "ember-power-calendar" but you don't have any of the required meta-addons to make it work, like 'ember-power-calendar-moment' or 'ember-power-calendar-luxon'. Please add to your app.`,
    );
  }
})();

Actually the projects that I maintain rely on ember-power-calendar-date-fns adapter and the reason is simple: we migrated away from Moment a few months ago because they consider themself a legacy project now and date-fns is actually well maintained and fits our needs.

I don't think that break the compatibility with external date libraries is a good solution, especially if one of the two available adapters is for a library that's no longer supported. In #300 someone mentioned also an usage with dayjs for example.

Maybe the old ember-power-calendar-utils handling was really ancient and its removal is good, I don't know the reason of the removal, but create a new interface to use other adapters should be considered important now, and I hope to not be obliged to be stuck with 0.19.0 forever because of the compatibility with date-fns.

@bertdeblock
Copy link
Contributor

The previous implementation was very vague and implicit, so it could very well be that the author of #329 didn't know other date back-ends like ember-power-calendar-date-fns were supported. I think it would be even better if consumers of this addon have to register the desired date back-end themselves, like so:

// app/app.js

import { setDateUtils } from 'ember-power-calendar';
import { dateUtils } from 'ember-power-calendar-date-fns';

setDateUtils(dateUtils);

This way:

  • ember-power-calendar doesn't need to "guess" which date back-end the consumer wants
  • The date back-end dependency is actually imported and used in the app itself, now it's just an entry in the app's package.json file and it's not immediately clear how / where it's used
  • Consumers can even write their own in-app date back-end if they want, or override a specific date util if needed (e.g. to temporarily fix an issue)
  • It would also fix Impossible to use inside a yarn workspace #334

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 1, 2023

we can add also ember-power-calendar-date-fns in the switch case, but this package is very old and outdated... and must get an update, becaus it is using old dependency like ember-auto-import...
in additional we need to export the addon with real name instead of ember-power-calendar-utils.

I will try to make a PR in thr next days here and in ember-power-calendar-date-fns

@bertdeblock
Copy link
Contributor

I would still consider using an API to set the date utils, compared to hardcoding another addon in the switch. The current setup forces people to use one of the hardcoded addons and disallows them from using any other Date library. What if, at some point, someone wants to create a fork and publish @some-scope/ember-power-calendar-date-fns? They won't be able to use it with the current setup, because it's not a supported addon, and adding forks to the switch doesn't seem like a good idea either.

@ErikPelli
Copy link
Author

Maybe a completely explicit function is a good way to let the user choose its date adapter but a kind of automatic assignment when there is an imported date adapter is still a nice thing to have.

Chartjs seems to have a good implementation for date libraries (I'm sure they support luxon and date-fns, didn't check for others) and they have a documented interface to make your own if you want to.

Maybe their API interface for external date adapters can be a good starting point to define the interface for date adapters in this project.

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 1, 2023

Yes API is a possible option, if it is embroider safe and addon v2 compatibile. The only problem is, that we never know which meta packages exists and if they are working after changes in this repo
For the current three packages we can add tests so we know if there are compabible

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 1, 2023

@ErikPelli could you provide any link?

@ErikPelli
Copy link
Author

ErikPelli commented Sep 1, 2023

@ErikPelli could you provide any link?

https://github.com/chartjs/Chart.js, It's vanilla JavaScript though

Adapters:

Internally they call an explicit set function on chartjs _adapters private field. A bit weird but it works and the API seems clean, maybe they do this to allow only compatible libraries to override the adapter (but it can be changed to an explicit global function as @bertdeblock suggests).

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 1, 2023

maybe we can use initializer to register the external package, so the consumer app doesn't need to make any setup in app.js...

@ErikPelli
Copy link
Author

Yep, since this is an ember-specific addon, it makes completely sense to use the available features instead of standard js like they do

@bertdeblock
Copy link
Contributor

The only problem is, that we never know which meta packages exists and if they are working after changes in this repo

This isn't any different from not knowing which apps are using this addon, and if those apps still work after changes to this addon. That's why SemVer exists, to correctly communicate changes to the public API towards consumers (both apps and addons).

Also, initializers are not really a recommended feature anymore.
It's even possible that consuming applications don't even use ember-load-initializers.

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 2, 2023

Is there really necessary to use ember-load-initializers?

My plan was to use application initializer, which provides ember ot of the box. https://api.emberjs.com/ember/5.2/classes/Application/methods/initializer?anchor=initializer

@bertdeblock
Copy link
Contributor

Initializers only run automatically because apps have ember-load-initializers installed.
It's in the default app blueprint => https://github.com/ember-cli/ember-cli/blob/master/blueprints/app/files/package.json#L87.

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 3, 2023

@bertdeblock Okay thanks, is there any RFC, which tells us, that initializers are outdated and not anymore recommanded?

I'm not really happy that app comsumers must do always manually steps to get run the addons, but if this is the new, recommanded way to go we can also change it.

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 3, 2023

@bertdeblock created a PR, like you described (without initializer).
The changes i haved added atm in moment & luxon package cibernox/ember-power-calendar-moment#56 / cibernox/ember-power-calendar-luxon#31
Now we need only to updatethe outdated ember-power-calendar-date-fns

import { registerDateLibrary } from 'ember-power-calendar';
import DateUtils from 'ember-power-calendar-moment';

registerDateLibrary(DateUtils);

@Techn1x
Copy link
Contributor

Techn1x commented Sep 4, 2023

Just chiming in to say I've also hit this issue when updating to 0.20.2, I use the date-fns addon as well.

@mkszepp
Copy link
Collaborator

mkszepp commented Sep 4, 2023

@mkszepp mkszepp mentioned this issue Sep 4, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants