-
Notifications
You must be signed in to change notification settings - Fork 19
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
Comments
@StefanKern All of these features is also included in my PR to vendure's repo, here #5. |
Thanks @colsen1991 will be using your version until the situation is clear |
Hi chaps, In any case, now I'm making my way through the PRs and will issue a new version today. |
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! |
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 |
@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:
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? |
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. |
you can try it out at npm old commit from Biesbjerg that originally introduced the feature: commit that removed it: commit where i re-introduce it: 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) |
@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? |
@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. |
@michaelbromley branch for marker lib should be ready: https://github.com/Husbanken/ngx-translate-extract-marker/tree/chore/move-to-vendure-ecommerce :) |
@colsen1991 I see you created the fork but the librarary "@vendure/ngx-translate-extract-marker" is not released yet, isn't it? |
Hi all! |
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 👍 |
Can I somehow help you with it? |
@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! |
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 :) |
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 :) |
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. So after trying it out for way to long, I would keep the two projects. |
With the recent release of ngx-translate@16, the core library now includes a built-in marker function: I suggest updating the documentation to point to this direction. For simple use cases, the built-in marker function can be highlighted as the preferred approach. For more advanced scenarios where a marker pipe or directive is required, the documentation could still reference the implementation by @colsen1991. |
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:
Are there any plans to take over this library too?
The text was updated successfully, but these errors were encountered: