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

refactor: change dynamic import to work with bundlers #1905

Merged
merged 2 commits into from Dec 12, 2023

Conversation

atodd
Copy link
Contributor

@atodd atodd commented Dec 7, 2023

Moving the './' path to the require statement allows bundlers to find the api.js and stub_api.js files

Description

Move path for api dynamic import to require statement in initApi.

Related Issues

Closes #1904

Moving the './' path to the require statement allows bundlers to find the api.js and stub_api.js files
@CLAassistant
Copy link

CLAassistant commented Dec 7, 2023

CLA assistant check
All committers have signed the CLA.

@jsumners-nr
Copy link
Contributor

👋 thank you for the contribution. It looks like it should work correctly, but Node v18.19.0 has introduced an issue in our CI. I have a PR out to an upstream module solve that problem and we can hopefully get it dealt with soon.

@jsumners-nr
Copy link
Contributor

The fix has been landed in our repo. @atodd can you rebase your branch please?

@bizob2828 bizob2828 assigned bizob2828 and jsumners-nr and unassigned bizob2828 Dec 11, 2023
@bizob2828
Copy link
Member

@atodd just for my own edification can you explain your fix? Your change just moves the ./ to be hardcoded in the require. Does your bundler configuration look for something ./${apiPath}? If so, why can't it just look for apiPath? Is it because it's too ambiguous?

@atodd
Copy link
Contributor Author

atodd commented Dec 11, 2023

@bizob2828 My understanding (at least for the bundlers I have used, mainly Vite and Webpack) is that require($apiPath) is too ambiguous because the bundlers can not determine the value of $apiPath. However, adding the ./ tells the bundler what directory a file will be dynamically imported from.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf22ae5) 96.88% compared to head (cfdf2a5) 96.88%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1905   +/-   ##
=======================================
  Coverage   96.88%   96.88%           
=======================================
  Files         209      209           
  Lines       39872    39872           
=======================================
  Hits        38631    38631           
  Misses       1241     1241           
Flag Coverage Δ
integration-tests-16.x 78.70% <100.00%> (-0.02%) ⬇️
integration-tests-18.x 78.96% <100.00%> (-0.02%) ⬇️
integration-tests-20.x 78.97% <100.00%> (-0.02%) ⬇️
unit-tests-16.x 90.88% <100.00%> (ø)
unit-tests-18.x 90.87% <100.00%> (ø)
unit-tests-20.x 90.87% <100.00%> (ø)
versioned-tests-16.x 73.79% <ø> (-0.03%) ⬇️
versioned-tests-18.x 73.79% <ø> (-0.03%) ⬇️
versioned-tests-20.x 73.80% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jsumners-nr jsumners-nr merged commit ee81429 into newrelic:main Dec 12, 2023
22 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Dec 12, 2023
@atodd atodd deleted the feat/improve-bundling branch December 13, 2023 16:16
@github-actions github-actions bot mentioned this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

Modify the dynamic import of api.js and stub_api.js to better work with bundlers
4 participants