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

Update ora to pick up ansi-regex security update. Fix #291 #292

Merged
merged 1 commit into from
Nov 27, 2021
Merged

Update ora to pick up ansi-regex security update. Fix #291 #292

merged 1 commit into from
Nov 27, 2021

Conversation

Pike
Copy link
Contributor

@Pike Pike commented Nov 22, 2021

Switching the import of ora from require to import as the only version with the ansi-regex update requires ESM.

I didn't want to impose that on plop, but the async-await should be OK for all supported versions of node, I think.

I'm dropping the export of the spinner, I have no idea what implications that has.

Switching the import of ora from require to import as the only
version with the ansi-regex update requires ESM. I didn't want to
impose that on plop, but the async-await should be OK for all
supported versions of node, I think.
@crutchcorn
Copy link
Member

Yeah, this is super helpful. We'll need to add back the export (simply because it impacts wrapper utils)

Unfortunately, it will be a major change, since we support older versions of Node. I'll be working on integrating a few other things and making a release alongside in the next day or two.

https://github.com/plopjs/plop/blob/master/package.json#L56-L58

@Pike
Copy link
Contributor Author

Pike commented Nov 22, 2021

Yeah, it's pretty unfortunate that we'll need a major version bump for this. I looked into ora a bit, and didn't see any indications that the author would be happy to release a 5.4.x version. And the ora-classic fork doesn't have the dependency fix either, AFAICT.

Personally, I doubt we'll have issues with updating either way, through esm or with dropping the progress export. I didn't see a chance to keep the export w/out esm, due to the need for an async import, which can't do a sync export.

@crutchcorn
Copy link
Member

Yeah, totally agree. Just that this will be the first major update of Plop since 2017 and I want to make sure we keep things running smooth (and also, since I have tons of time tonight, that I can include some other changes that will make the major update sting less)

I deeply appreciate your help on this one and thoughts on it!

@crutchcorn
Copy link
Member

Just wanted to provide some transparency here onto what's happening with this issue/PR

First, some context: this is not the only thing that's needing changing in order to fix this problem. node-plop likewise has an ESM dep requirement that's forcing me to migrate there as well. But, of course, that means that I need to make plop, you know, work /migrate to ESM

That means that I'm implementing support for #260

But it also means that I need to go and update all of the 127 tests (the test code is not great on reflection)

I've spent 13 hours in the past two days to get this done (on top of my full-time job) and am 90% there. plop itself has tests, works great, and is ESM mode now. node-plop on the other hand, while the API seems to work, there are still 7 failing tests I'm working on solving

After that, it's a matter of documentation and a few other small things (roadmap here: plopjs/node-plop#211)

@crutchcorn crutchcorn merged commit d152d6d into plopjs:master Nov 27, 2021
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.

None yet

2 participants