-
-
Notifications
You must be signed in to change notification settings - Fork 166
Draft: Point Pulsar's first-party dependencies to NPM packages #1363
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
Draft: Point Pulsar's first-party dependencies to NPM packages #1363
Conversation
…to point to their NPM-published versions instead of random GitHub commits.
…and regenerate `yarn.lock` from scratch. (This may be a mistake; we'll see.)
|
Good news! After addressing some show-stopping and extraneous failures, I've now got this down to the expected number of CI failures. The remaining handful of failing CI specs are known to be flaky and all pass for me locally on macOS, so I'm taking this out of draft. |
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.
I can confirm the CI failures are similar to those on the tip of the target branch for this PR. (And yeah they do seem flaky based on the differences in reruns.)
A lot of linter-generated whitespace changes from function() to function (), but eh... Oh well.
I am NOT familiar with the TypeScript stuff, but it seems small and bugfixy ... ? Rather than too ambitious or radical. At a glance looks okay?
Not sure what motivated regenerating yarn.lock entirely, but if the result works, then I suppose it's alright.
Not a huge amount is changed here other than bumping deps. Hopefully those dep bumps turn out to be in good shape, we'll see. But yeah, the return of semver and actual dependency version numbers in package.json, would you look at that.
Not my most informed review ever, but the PR doesn't look too crazy...
LGTM.
|
I regenerated
As a sanity check, I did I might revert the regeneration of |
|
Nah! Tried it and it still didn't work. Despite the only references to |
53c7da5 to
a71f0c0
Compare
ccdbc90
into
updated-latest-electron
This PR has one extra commit on top of the current HEAD of
updated-latest-electron: one which updatespackage.jsonto use the published-to-NPM versions ofsuperstring,text-buffer,pathwatcher, et al., and updates the imports themselves to reference the namespaced package names.When I tried this locally earlier today, it revealed a couple of mistakes I'd made in the publishing process. But once I fixed those, everything seemed to work. So the next step is to open this PR and see if the binaries generated by CI also work out of the box.
I have procrastinated on one last item: updating the(Edit: Fixed this, I think.)githubpackage to use a newer version ofwhats-my-linethat points to@pulsar-edit/superstringrather thansuperstring. For now I “fixed” this by using theresolutionssection ofpackage.jsonsuch that any attempts to importsuperstringbywhats-my-lineinstead get@pulsar-edit/superstring. But I'll tackle thegithubpackage tomorrow or Tuesday.Testing
If you're on a platform that can use the CI-generated binaries (i.e., not Apple Silicon or ARM Linux), please download the artifacts and give it a try! If nothing explodes, that'll be a very positive sign.