Skip to content

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Oct 24, 2024

Also drop the cross-var dep, now unused.

…gs in contrib repo

Also drop the cross-var dep, now unused
@codecov
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.85%. Comparing base (9a20e15) to head (2ee3088).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2501   +/-   ##
=======================================
  Coverage   90.85%   90.85%           
=======================================
  Files         159      159           
  Lines        7853     7853           
  Branches     1622     1622           
=======================================
  Hits         7135     7135           
  Misses        718      718           

@trentm
Copy link
Contributor Author

trentm commented Oct 24, 2024

A little story about cross-var, if you would like a little crazy in your life:

git clone [email protected]:open-telemetry/opentelemetry-js.git this-src-is-crazy
cd this-src-is-crazy
npm ci
npx cross-var echo hi

Does that fail with something like this?

/Users/trentm/tmp/this-src-is-crazy/node_modules/babel-core/lib/transformation/file/options/option-manager.js:328
        throw e;
        ^

ReferenceError: Unknown plugin "transform-runtime" specified in "/Users/trentm/tmp/this-src-is-crazy/node_modules/babel-plugin-transform-regenerator/node_modules/regenerator-transform/package.json" at 0, attempted to resolve relative to "/Users/trentm/tmp/this-src-is-crazy/node_modules/babel-plugin-transform-regenerator/node_modules/regenerator-transform" (While processing preset: "/Users/trentm/tmp/this-src-is-crazy/node_modules/babel-preset-es2015/lib/index.js")
    at /Users/trentm/tmp/this-src-is-crazy/node_modules/babel-core/lib/transformation/file/options/option-manager.js:180:17
...

We use cross-var in the core repo so that we can have the odd npm script that uses $foo for envvars, e.g. cross-var echo this is my $SOMEVAR, and it'll do the right thing for Windows (transform '$FOO' to '%FOO%' and spawn it).

Turns out the author wanted to use ESM code, but unfortunately was living in 2017 when this was written (before current versions of Node.js supported ESM). "No problem," they said, "I'll use Babel to down compile to JS that the current node can run." At runtime. Everytime. "And I'll use the handy babel-register command to do it."

babel-register installs a require hook that attempts to transpile every file that is loaded.
"I don't want that," says the developer, "so I will use its only option to only transpile the files in my src/ dir"

https://github.com/elijahmanor/cross-var/blob/e5aaa1d2de34f6b18a1251ad774c3ec41a219255/index.js#L3-L6

require( "babel-register" )( {
    ignore: false,
    only: /src/
} );

Unfortunately that means that any match for "src" in the full path is a hit -- and "src" is in the top dirname I specified above. And babel blows up on ".../this-src-is-crazy/node_modules/babel-plugin-transform-regenerator/node_modules/regenerator-transform/lib/index.js" for reasons I didn't get in to.

(In my personal case, I sometimes work on a clone of opentelemetry-js.git and opentelemetry-js-contrib.git in my ~/src/ directory. Boom.)

If the above was changed to:

require( "babel-register" )( {
    ignore: false,
    only: /cross-var\/src\/index.js$/
} );

it would be fine.
But cross-var hasn't been updated in 7y.
There is a PR for this: elijahmanor/cross-var#18
And an npm-published fork with the languishing PR: https://www.npmjs.com/package/cross-var-src-patch

I would like to just remove cross-var from our repos.
This PR does so for the contrib repo, where cross-var usage snuck back in with the returning propagator-aws-xray package.

@github-actions
Copy link
Contributor

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

@trentm trentm merged commit c260e26 into open-telemetry:main Oct 31, 2024
23 checks passed
@trentm trentm deleted the tm-the-cross-var-the branch October 31, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants