Skip to content

fix: delete broken objectFromEntries#1306

Merged
erights merged 1 commit intomasterfrom
markm-deprecate-objectFromEntries
Oct 3, 2022
Merged

fix: delete broken objectFromEntries#1306
erights merged 1 commit intomasterfrom
markm-deprecate-objectFromEntries

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented Sep 30, 2022

commons.js contains its own ponyFill (aka shim, polyfill) of Object.fromEntries because it was relevant at the time. It no longer is. Fortunately it was dead code anyway because it was only exported (as fromEntries) when on a platform on which it is absent. Thus deleting it causes no transition cost. We can avoid the deprecation dance. By this criteria of course, fixing this is also low priority.

The reason to bother getting rid of it is that it uses assignment semantics to initialize the object it creates and returns. The resulting problems explained at #1303 this has some terrible problems do apply to this code as well.

@erights erights requested a review from kriskowal September 30, 2022 22:59
@erights erights self-assigned this Sep 30, 2022
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 30, 2022

Hmmm. https://github.com/endojs/endo/actions/runs/3162163428/jobs/5148508233 shows a platform compat failure, presumably because it is testing a platform only enough to still not have its own fromEntries. If we decide we still support such platforms, then the code is not dead and its assignment problems (like #1303) should be fixed.

If we decide we no longer support such platforms, we should remove that test, or at least make not Required.

@kriskowal
Copy link
Copy Markdown
Member

@erights Consider review and rebase on #1308

I’m in favor of allowing Node.js 10 vanish into history.

@erights erights force-pushed the markm-deprecate-objectFromEntries branch from 09ba3e4 to b463d77 Compare October 1, 2022 01:01
@erights erights changed the base branch from master to kris-goes-up-to-eleven October 1, 2022 01:02
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Oct 1, 2022

@erights Consider review and rebase on #1308

I’m in favor of allowing Node.js 10 vanish into history.

Rebased, thanks!

@kriskowal kriskowal force-pushed the kris-goes-up-to-eleven branch from 44f9480 to 17c44e3 Compare October 1, 2022 01:33
Copy link
Copy Markdown
Member

@kriskowal kriskowal 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. I’m surprised that we no longer use fromEntries anywhere, but I trust the tests.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Oct 1, 2022

Thank you. I’m surprised that we no longer use fromEntries anywhere, but I trust the tests.

Look again. I'm still exporting fromEntries, but now doing so like a normal static method.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Oct 1, 2022

Huh. But maybe we don't actually import it anyway. I'm just as surprised.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Oct 1, 2022

It is imported by console.js so I'm going to leave the new export as is.

@turadg turadg force-pushed the kris-goes-up-to-eleven branch from 17c44e3 to 92194e3 Compare October 3, 2022 16:23
Base automatically changed from kris-goes-up-to-eleven to master October 3, 2022 16:29
@erights erights force-pushed the markm-deprecate-objectFromEntries branch from b463d77 to b9bb49f Compare October 3, 2022 17:03
@erights erights merged commit d83be67 into master Oct 3, 2022
@erights erights deleted the markm-deprecate-objectFromEntries branch October 3, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants