Skip to content

fix: Use file path imports#318

Merged
wemeetagain merged 4 commits intoChainSafe:masterfrom
FrederikBolding:use-explicit-imports
Apr 21, 2023
Merged

fix: Use file path imports#318
wemeetagain merged 4 commits intoChainSafe:masterfrom
FrederikBolding:use-explicit-imports

Conversation

@FrederikBolding
Copy link
Copy Markdown
Contributor

@FrederikBolding FrederikBolding commented Apr 19, 2023

Motivation

The goal of this PR is to stop relying on package.json exports, as it may break older build tools such as Browserify.

Description

Uses explicit imports for all imports that were previously using exports aliases.

Steps to test or reproduce

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 19, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 19, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@FrederikBolding FrederikBolding changed the title Use explicit imports fix: Use explicit imports Apr 19, 2023
@FrederikBolding FrederikBolding marked this pull request as ready for review April 19, 2023 10:11
@FrederikBolding FrederikBolding requested a review from a team as a code owner April 19, 2023 10:11
@FrederikBolding FrederikBolding mentioned this pull request Apr 19, 2023
Closed
@FrederikBolding
Copy link
Copy Markdown
Contributor Author

FrederikBolding commented Apr 20, 2023

@g11tech My bad, tests should work now.

@FrederikBolding FrederikBolding mentioned this pull request Apr 20, 2023
Merged
@legobeat legobeat mentioned this pull request Apr 21, 2023
Merged
g11tech
g11tech approved these changes Apr 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm, waiting for @wemeetagain to give a look, @acolytec3 / @holgerd77 if you also want to review

@acolytec3
Copy link
Copy Markdown
Contributor

acolytec3 commented Apr 21, 2023

I'll run our browser tests against it and see if we can remove our aliases in karma and let you know.

@paulmillr
Copy link
Copy Markdown

paulmillr commented Apr 21, 2023

let's merge it please, it's a blocker for some apps

@wemeetagain wemeetagain changed the title fix: Use explicit imports fix: Use file path imports Apr 21, 2023
@wemeetagain wemeetagain merged commit f459e92 into ChainSafe:master Apr 21, 2023
@FrederikBolding FrederikBolding deleted the use-explicit-imports branch April 21, 2023 13:28
@wemeetagain wemeetagain mentioned this pull request Apr 21, 2023
Merged
@acolytec3
Copy link
Copy Markdown
Contributor

acolytec3 commented Apr 21, 2023

I checked this against our current master branch and the module resolution issues we had in our browser tests are no longer an issue. Can't wait to update our code with this.

@g11tech
Copy link
Copy Markdown
Contributor

g11tech commented Apr 21, 2023

thanks @acolytec3

@paulmillr @FrederikBolding i have release the lib: #319, pls try the new versions

@FrederikBolding
Copy link
Copy Markdown
Contributor Author

FrederikBolding commented Apr 21, 2023

@acolytec3 I have a branch ready for when the new version goes up.

@g11tech I don't see it published on NPM quite yet.

@wemeetagain
Copy link
Copy Markdown
Member

wemeetagain commented Apr 21, 2023

CI is still running, give it a few minutes

@FrederikBolding FrederikBolding mentioned this pull request Apr 21, 2023
Merged
This was referenced May 2, 2023
Merged
Closed
Closed
@matthewkeil matthewkeil mentioned this pull request Jan 15, 2025
Closed
@wemeetagain wemeetagain mentioned this pull request Jan 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wemeetagain wemeetagain wemeetagain approved these changes

+1 more reviewer

@g11tech g11tech g11tech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@FrederikBolding @CLAassistant @acolytec3 @paulmillr @g11tech @wemeetagain