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

deps: Update superstring to Pulsar's fork #5

Merged
merged 14 commits into from
Nov 9, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Oct 23, 2023

Identify the Bug

The superstring dependency of this package can't build on NodeJS 18 or newer. (Also affects the github package that depends on this one.)

Description of the Change

Update superstring to the tip of pulsar-edit org's fork of superstring's default branch (https://github.com/pulsar-edit/superstring)

Alternate Designs

N/A (none were considered)

Possible Drawbacks

None anticipated, the workaround for building on Node 18 was done in a backward-compatible manner by detecting older NodeJS and using the original prior code on those NodeJS versions.

Verification Process

CI should pass, this package should remain functional with this bump.

Release Notes

deps: Update superstring to pulsar's fork

Should allow building on at least NodeJS 18.x
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Oct 23, 2023

I would have accidentally opened this pull request against atom/whats-me-line if it weren't archived.

GitHub's UI about PRs pointing to forks could be a little less presumptuous about pointing PRs against the parent repo... grumble grumble grumble.

ANYHOW, back on topic: I kind of want to add macos-latest and windows-latest to the test matrix here??

@DeeDeeG DeeDeeG marked this pull request as draft October 23, 2023 08:55
@confused-Techie
Copy link
Member

If it's of any help, thought I'd point out where I've seen this error message before.
It was actually right over on our discussions.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Oct 24, 2023

So, yeah, the win-iconv submodule is not being fetched by recent npm. npm/cli#2774

Broken in npm 7.x, not restored. Docs still claim this is supported, but it is not actually working: https://docs.npmjs.com/cli/v10/commands/npm-install

If the repository makes use of submodules, those submodules will be cloned as well.

(Not true as of npm 7.)

(Not to put too fine a point on it, but as far as I can tell, this change was unplanned, unannounced and undocumented. Sheer oversight, and effectively willy-nilly breaking changes. In pursuit of cleaning up the codebase, refactoring, and faster performance. (And apparently done (by accident) during the pursuit of a better dependency resolution algorithm, which is at least a nice goal to be pursuing, admittedly.) All this in the premier dependency management and build tool in the NodeJS ecosystem. Go figure. If anyone wonders what upset me so much that I have never gotten over npm 7, here you go, one more reason.)

EDIT to add: I think they were coming from the right place, but they were way, way, way too bold about the changes, and way, way, way too confident about the lack of disruptive breaking changes they supposedly didn't have in npm 7... And they didn't have adequate [ability to] outreach to npm users to spot breaking changes before they became de-facto maintainer-blessed changes as of npm 7, since they didn't want to churn and introduce what could be construed as further breaking changes by fixing them after the semver-major stable npm 7.0 had already been released...

I consider the whole thing botched as regards unintended breaking changes, but since I'm the one who's complaining the loudest, it seems to have worked out fine for most users. It just shook out especially extremely badly for an Atom contributor who was dealing with a lot of legacy tech...

This reverts commit c571758.
Should fix the "missing win-iconv submodule" situation,
which breaks the Windows builds.
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Oct 24, 2023

We may want to just commit the win-iconv files into superstring repo. It's 11 files, no subdirectories, it's public domain, and they're like ~87-116 KB, depending on how you measure it, on-disk. The latest change was in 2016.

I am thinking it's PR time to just commit the files instead of dealing with submodule headaches.

UPDATE: Here it is (pulsar-edit/superstring#6). A PR to vendor win-iconv into our superstring fork without using submodules.

Workaround for a weird Yarn v1.x issue,
which only happens when global npm is v9.7.2 or newer.

See pulsar-edit/ppm#101 for an explanation.
This revision makes superstring easily buildable on Windows as a git
dependency or tarball URL dependency, by vendoring in the 'win-iconv'
files as regular files, not as a submodule.

npm and Yarn do not necessarily install submodules of git repos as of
their latest versions. And tarballs wouldn't be generated with the
submodule content, as far as I know.

So, doing it this way is much more broadly compatible with how the
package managers tend to work these days.
@DeeDeeG DeeDeeG marked this pull request as ready for review November 8, 2023 22:03
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Nov 8, 2023

This is finally ready!

After the win-iconv situation was sorted out over at superstring repo, and CI at this repo was both enhanced + fixed slightly for this PR, things are looking good here!

(I often like to clean up my WIP branches, this has a bunch of reverts in it, but I dunno if I prefer to clean them up and rebase, or leave the evidence of all the things I had to try to get here. Anyhow, just opening it back up for now! Reviews welcome!)

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Lets get this merged! Super excited to see we were finally able to get this one going!

Thanks a ton for following through!

Now with all these changes, it's just time to get them all upstream, and be happy with our smaller binaries, and more up to date everything lol

@confused-Techie confused-Techie merged commit b828ad0 into master Nov 9, 2023
6 checks passed
DeeDeeG added a commit that referenced this pull request Dec 13, 2023
…pulsar-fork"

This reverts commit b828ad0, reversing
changes made to 2568f93.
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