-
Notifications
You must be signed in to change notification settings - Fork 400
Add globalThis
-preserving Import Path
#875
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
Add globalThis
-preserving Import Path
#875
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup here!
Doesn't this line in package.json
need to change so that resolving the ESModule resolves this new index.js that doesn't pollute the global namespace?
Line 11 in d1c9f88
"module": "./dist/RosLib.js", |
TLDR: I'm proposing something much smaller. I'm trying to make this a no-brainer tiny PR that for-sure breaks nothing. I'd be happy to open a separate PR for the change you're talking about, which is a much bigger change. Why does this small change work for me?
Why would I make a separate PR for the package.json change?
If there needs to be some debate/discussion/delay around # 2, then I'd ask that this PR be considered/merged as-is (no-breakage) and then I can open up a second (possibly-breaks-things) PR for changing the npm endpoint. |
Just wanted to check back in on this since it should be a really small non-breaking change @EzraBrooks |
src/RosLib.js
Outdated
@@ -1,28 +1,11 @@ | |||
/** | |||
* @fileOverview | |||
* @author Russell Toris - [email protected] | |||
* @author Jeff Hykin - [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the original author here. You can still add yourself if you like (though personally I would only do that for major changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont worry, I'm perfectly fine with no mention.
However, it is a big deal to me how bad this looks on me (e.g. the diff looks like I edited-out the original author, which is not the story on my end). I renamed RosLib.js to index.js (hence author is maintained there) and then I made a new file. I wasnt sure on your policy for new files so tried to copy the header style and put my info.
Naturally git doesn't know I renamed RosLib.js to index.js so it looks like I edited the original. I probably should've checked the diff and swapped the headers.
Sorry for that confusion. Either way, I agree the original author should be at the top of both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I sort of noticed that when the "new file" in the diff had the former author's name. All good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Thank you for merging! |
Public API Changes
None
Description
This has no functional change for all existing users (ex: CDN). RosLib.js exports all the same API's as before and still sets
globalThis.ROSLIB =
.However, for users like myself who want to avoid polluting
globalThis
, this PR providessrc/index.js
.NOTE: to avoid code duplication
src/index.js
consolidates the importssrc/RosLib.js
re-exports everything from index.js, and then additionally sets ROSLIB