Skip to content
This repository has been archived by the owner on Jun 7, 2022. It is now read-only.

Adds @woocommerce/tracks package #5017

Merged
merged 13 commits into from Aug 20, 2020
Merged

Adds @woocommerce/tracks package #5017

merged 13 commits into from Aug 20, 2020

Conversation

danielbitzer
Copy link
Contributor

Fixes #5016

Moves the functions from lib/tracks to a @woocommerce/tracks package so other Automattic extensions can use them.

Detailed test instructions:

  • Test that tracks events are still firing as normal

Changelog Note:

Dev - Added @woocommerce/tracks package

cc: @jconroy

Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @danielbitzer ! This is testing well and code looks good, including changes to READMEs. I found no instances of "lib/tracks" in the codebase too.

After a few ongoing PRs get merged, we can publish packages soon including this one.

@danielbitzer
Copy link
Contributor Author

Thanks for the review @psealock!

After a few ongoing PRs get merged, we can publish packages soon including this one.

Cool! Can you let me know when the package is published so I can P2 and we can start using it?

@danielbitzer danielbitzer merged commit 70f45da into main Aug 20, 2020
@danielbitzer danielbitzer deleted the add/wc-tracks-package branch August 20, 2020 04:59
@timmyc
Copy link
Contributor

timmyc commented Aug 20, 2020

Really great addition @danielbitzer - thanks for making this happen.

@nerrad
Copy link
Contributor

nerrad commented Aug 20, 2020

Awesome!

@psealock
Copy link
Collaborator

Can you let me know when the package is published

Absolutely. Here are the remaining tasks I can identify. cc @timmyc in case you have any others in mind. These are all being actively worked on.

#3387
#3381
#3382
#5018

@jconroy
Copy link
Member

jconroy commented Aug 20, 2020

Great work @danielbitzer 👏

@haszari
Copy link
Member

haszari commented Aug 24, 2020

Now that WooCommerce Admin is included in core, do we need to publish this as a package? Maybe I'm missing something ..

In my current Woo install, I don't have WooCommerce Admin installed, and window.wcTracks.recordEvent is available for use. You'd need to require a recent version of WooCommerce, so there might be some limitations here (though I think it's been included for a while now).

Screen Shot 2020-08-24 at 1 40 21 PM

If there's added value (e.g. util functions) in wc-admin that's useful, maybe it needs to be exposed as part of core.

@danielbitzer
Copy link
Contributor Author

@haszari WC Admin's lib/tracks had some small extra features that could be used in other extensions:

  • Debugging, personally I find this super useful
  • queueRecordEvent() is also useful for other extensions
  • We can add more utility functions or maybe even a custom hook in the future and all extensions can use it
  • Also I think it's a little nicer to access external code via an import rather than a via a window prop

@haszari
Copy link
Member

haszari commented Aug 24, 2020

Aha, thanks for clarifying @danielbitzer ! Note the main reason I'm asking these questions is to better understand if we should use window.wcTracks vs using the package - i.e. the "pitch" for the package. I see you have a nice readme for the package 🙌, maybe we could add something there about the value add for using package over core.

Debugging, personally I find this super useful

Agreed, that makes sense.

queueRecordEvent() is also useful for other extensions

Is this going to be available as part of core in future? Definitely seems useful!

We can add more utility functions or maybe even a custom hook in the future and all extensions can use it

I was wondering if util code was a motivator here. I'm curious whether this could be included in core or whether it's better to make available as a package.

Also I think it's a little nicer to access external code via an import rather than a via a window prop

Assuming the code is the same, is it redundant/extra bytes to consume via package when it's on a global? Or, put another way, should we remove the global in favour of using the package? I agree a package makes it easier to get the latest APIs/code (i.e. known version) so that's an advantage.

cc @timmyc @psealock

@joshuatf
Copy link
Contributor

Also I think it's a little nicer to access external code via an import rather than a via a window prop

This and the idea that we can iterate on things in an external package prior to merging into core I believe are the main reasons for keeping it under the namespaced @woocommerce/tracks package.

There are more features we can add in here for queuing prior to navigation so that we're not missing tracks due to race conditions.

Is this going to be available as part of core in future? Definitely seems useful!

Someone can correct me if I'm wrong, but I think the answer is "yes." The current model seems to be:

  • Develop in WCA until stable
  • Package in core
  • (Eventually?) merge to core.

Or, put another way, should we remove the global in favour of using the package?

I'd be in favor of this, but I imagine some legacy code would need to be cleaned up first. I'm not sure if the consuming scripts in core are capable of using ES6 imports or not yet.

@timmyc timmyc added this to the 1.6 milestone Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert lib/tracks to a @woocommerce/tracks package
7 participants