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

Are you going to take over "biesbjerg/ngx-translate-extract-marker" too? #4

Open
StefanKern opened this issue Jan 23, 2023 · 19 comments

Comments

@StefanKern
Copy link

Thank you for taking over the library from biesbjerg.

He also made a second library, which provides a marker to directly extract translations from a component by decorating it with a marker.
--> https://github.com/biesbjerg/ngx-translate-extract-marker

This library did not receive any updates in three years too, seems to be working fine, but is still based on View Engine. This results in a warning when installing it:

Processing legacy "View Engine" libraries:
- @biesbjerg/ngx-translate-extract-marker [es2015/esm2015] (https://github.com/biesbjerg/ngx-translate-extract-marker.git)

Are there any plans to take over this library too?

@colsen1991
Copy link

@StefanKern
I do not wish to impose on or steal Vendure's thunder in taking over the biesbjerg's great work, but given a sudden need to upgrade both extract and extract-marker at our company I've recently forked and updated both of these libraries to support Ivy (thanks to already existing PRs) as well as some other features such as marker pipe and directive. So if you're in a rush you can look up those in the meantime while this issue is being resolved; extract, marker.

All of these features is also included in my PR to vendure's repo, here #5.

@tgangso
Copy link

tgangso commented Feb 13, 2023

Thanks @colsen1991 will be using your version until the situation is clear

@michaelbromley
Copy link
Member

Hi chaps,
I'm really sorry - I had failed to set up notifications for this repo 🤦 so I wasn't aware of the activity here. A dropped ball for sure given the circumstances of this fork!

In any case, now I'm making my way through the PRs and will issue a new version today.

@michaelbromley
Copy link
Member

OK v8.0.5 is out now.

And this got me thinking - why is the marker function a separate package anyway? Is it because one is a dev dependency and the other is a regular dependency?

It seems an awful lot of overhead for what is essentially an identity function. What's a better way to do this? Open to suggestions here but in general my bias is to reduce dependencies wherever possible. Better for the consumers and maintainers!

@tmijieux
Copy link

tmijieux commented Mar 2, 2023

I think it was splitted to accomodate project dependencies because marker in meant to be included into final user projects while the extract is just tool required in "dev" requirement only. (that way you can avoid downloading the extract tool and all of its dependencies when you build you app in a automated ci for example)

I proposed a merge request to @colsen1991 's fork (not sure that it is available in the pull request here though ?) that re-added an old way of using the marker (that was in this project before but was removed at some point but my project was dependent on it and it was easier to re insert the feature rather than migrate my project, basically you can define your own custom function as a marker so that you do not have to import the one from the other repo and you just specify the name of the custom marker function on the command line with the --markeror -m option, also imho it is more versatile that the current method, because you can do what you want in the custom function))

@colsen1991
Copy link

@michaelbromley I don't know the history of the marker lib, but @tmijieux brings up a good point regarding tree shaking and the two libs being very different in terms of dev and runtime dependency. So i suggest the two libs stay separate. Also given that not everyone who uses extract needs the marker fn as extract also works with translate fn, pipe and directive.

So I suggest that either:

  1. @colsen1991/ngx-translate-extract-marker lives on (BTW, soon to be @husbanken/ngx-translate-extract-marker) as a standalone lib and I update my PR to redirect to it instead of @biesbjerg's marker lib
  2. or that you (Vendure) fork my fork of marker fn with its improvements and take over future maintanance

From what I can tell marker is just a no-op and I don't see many changes being introduced there in the future, especially since my fork now supports custom marker fn with the --marker and -m option. and a marker directive and pipe.

Either way I will update my PR to this repo to include the other improvents introduced in my fork, mark my fork as deprecated, redirect to yours (@vendure/ngx-translate-extract) and discontinue publishing it. Although I'll leave the published versions up on NPM for anyone who's added it as a dependency.

What do you guys think?

@michaelbromley
Copy link
Member

Hi @colsen1991

Long-term, I guess it makes sense for the extract & marker libs to both live in the Vendure org. Right now I don't want to hold up your work on your PR so I'd say we can leave it pointing to the husbanken version until I have a spare moment to fork and set up a new npm package.

I'm also interested in at some point exploring the proposal of @tmijieux about an API for a custom extract function.

@tmijieux
Copy link

tmijieux commented Mar 7, 2023

I'm also interested in at some point exploring the proposal of @tmijieux about an API for a custom extract function.

you can try it out at npm @colsen1991/ngx-translate-extract (or @aum_biosync/ngx-translate-extract)

old commit from Biesbjerg that originally introduced the feature:
Husbanken@bc5ce7e
Husbanken@daaebed

commit that removed it:
Husbanken@4fe3c43

commit where i re-introduce it:
Husbanken@f8d1279

note that the custom marker was originally replaced by "regular marker" and in the new commit, both feature are available and the -m option is a basically switch between both feature (only one at a time... but that could be changed as well if one want both simultaneously)

@colsen1991
Copy link

@michaelbromley yeah, makes sense for them to coexist under the same namespace, so let's talk after I've updated my PR (I'll let you know once its ready) and it has been merged and published.

And as @tmijieux mentioned the custom marker function has already been re-implemented in my fork, so if you want to test it out just follow his instructions. I'll look into including this option in the PR if you want?

@colsen1991
Copy link

@michaelbromley just updated the PR. Sorry for the mess, but I hoep you'll be able to parse/power through it :)

Also as mentioned there, I'm gonna prep a branch on my fork of ngx-translate-extract-marker to make it easier for you to take over that lib as well.

@colsen1991
Copy link

@michaelbromley branch for marker lib should be ready: https://github.com/Husbanken/ngx-translate-extract-marker/tree/chore/move-to-vendure-ecommerce :)

@StefanKern
Copy link
Author

@colsen1991 I see you created the fork but the librarary "@vendure/ngx-translate-extract-marker" is not released yet, isn't it?

@michaelbromley
Copy link
Member

Hi all!
I'm currently in a bit of a crunch to get Vendure v2 released next week. After that I'll find some time to address this issue.

@colsen1991
Copy link

No worries, @michaelbromley! And no, it's not published yet - I only created the branch to accomodate Vendure taking over :) So it should just be fork, merge, code review and publish on your end 👍

@StefanKern
Copy link
Author

Can I somehow help you with it?

@michaelbromley
Copy link
Member

michaelbromley commented Jun 13, 2023

@colsen1991 thanks for prepping things like that, makes my life much easier :)

@StefanKern my ideal setup would not be to have 2 separate git repos, but to have the marker package also published from this repo, making this a monorepo to keep everything together. If that is something you are interested in helping with, let me know and I'll make you a maintainer so you can work on that independently of me. If not, I will just form Christer's repo for now and publish from there.

ps Servus aus Wien!

@StefanKern
Copy link
Author

So you would like to merge the code into a single repository and publish both libraries from here? I would like to take a look at it in the next days and maybe I can be of help :)

PS: Grüße aus Wien zurück :)

@michaelbromley
Copy link
Member

So you would like to merge the code into a single repository and publish both libraries from here?

Yes exactly. Although the actual publishing step, I can take care of. Take a look and see if that's something you think you can do. If you need anything from me just let me know. For a start you should be fine just cloning this repo, but if you later want write access give me a shout :)

@StefanKern
Copy link
Author

Hey, I thought this would be a quick change, but then also did not really have the time for it. Also I'm no longer convinced it would be a good idea. ngx-translate-extract and ngx-translate-extract-marker user very different technologies. Merging them together then creates a pretty large package.json and makes the generation of the two packages more complex.

So after trying it out for way to long, I would keep the two projects.

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

No branches or pull requests

5 participants