Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Export package version #131

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

justindsmith
Copy link
Contributor

Exports the current package version from the root level. This allows
users of this module to read the version of the library that has been
loaded.

@justindsmith
Copy link
Contributor Author

The ocagent (#128) requests the core library version as part of it's payload. This PR exposes the core lib version in a cleaner manner so that it can be easily consumed by the exporters.

@bogdandrutu
Copy link
Contributor

Fix the build?

@justindsmith
Copy link
Contributor Author

Fix the build?

Yeah, I'm working on the fix for the tests now.

@justindsmith
Copy link
Contributor Author

I've tried to solve this in a way that keeps the overhead minimal (still one location to update the current version, namely the package.json) but also keeps the core package unbound to a node environment (relying on filesystem reads or any module from the core nodejs library). I’m now copying the package.json file into build directory after compilation, which means the compile time, build time, and import time all have access to the package.json at the same relative path.

/cc @kjin I believe the node 6 failure is just environmental. It works locally for me on node 6. Can you rerun the that workflow to see if it fixes itself?

Here are the failing logs:

{ Error: Command failed: npm run test
sh: 1: nyc: not found

@kjin kjin self-requested a review September 25, 2018 20:48
* limitations under the License.
*/

const pjson = require('../../package.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the pjson module which I believe recursively descends directories until it hits a package.json file. That way we don't need to copy it into the build directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only concern with that is that pjson uses the fs node library, which means that core would be node-only and wouldn't work in (for example) a browser context. Is that okay? My understanding architecturally was that core should be environment agnostic, with other packages (opencensus-nodejs) that are environment specific.

If that's not true, then yeah, something like pjson is a nice fit for this use case for sure and I could easily add that in.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. In that case, to avoid the file copy would it be possible to use a try-catch to descend either 2 or 3 directories, or hardcode '../../../package.json' instead? (I would prefer the former, but either works)

Justin Smith added 3 commits November 20, 2018 14:09
Exports the current package version from the root level. This allows
users of this module to read the version of the library that has been
loaded.
Tests were failing because the `package.json`'s relative location changes
between compile and test time (from "../../package.json" during `compile` and
"../../../package.json" during `test`). To resolve this, we now copy the
package.json into the build directory after compililation is done, which
keeps the relative location the same during compile, test, and deployment.
We do not need to handle two relative paths because both the packaged
lib and test suite run through the /build directory, which means we
can support a single path.

The opencensus-exporter-ocagent has been updated to support the newly
exported version.
@justindsmith
Copy link
Contributor Author

@isaikevych I've resurrected this older PR to get it cleaned up and pushed in. The main goal was to export the version of the core library in a way that a consumer could easily read it. This was necessary for the OCAgent exporter to report the version of the library per those specs.

I've incorporated the feedback from @kjin above into the review, but also went ahead and updated the ocagent exporter code to use the new version instead of loading it from the package itself.

Let me know if you have any questions!

@isaikevych isaikevych self-requested a review November 20, 2018 23:56
Copy link
Contributor

@isaikevych isaikevych left a comment

Choose a reason for hiding this comment

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

Thank you

@songy23
Copy link
Contributor

songy23 commented Nov 21, 2018

Should this PR be merged?

@justindsmith justindsmith merged commit 526023e into census-instrumentation:master Nov 21, 2018
@justindsmith justindsmith deleted the core-version branch November 21, 2018 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants