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

fix: allow [email protected] #30517

Closed
wants to merge 1 commit into from
Closed

Conversation

FezVrasta
Copy link
Contributor

The docs should be updated as well, so that users are instructed to install [email protected]. This will allow your consumers to not get notified about the deprecation notice included in the stable release branch of Popper.

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

Thanks, but I'd rather tackle all the related changes in one PR. Are you able to do it yourself or should do you want me to tackle it later?

@FezVrasta
Copy link
Contributor Author

I can try to update the docs, may you confirm v4-dev is the correct target branch?

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

Yup, assuming in master we'll manage to update to popper.js 2.x 🙂

@FezVrasta
Copy link
Contributor Author

Done, please let me know if you think I missed something.

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

I think we should update the Nuget files too.

BTW with this tag, I wonder how things will be with the semver operators for people. Say you release a 1.16.2-lts, this will require manual update, right?

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Apr 6, 2020

I don't plan to release any additional version on the 1.x development branch, unless security vulnerabilities are discovered (which is very hard, considered the scope of the library).

In that case, I'll release a new stable release (1.16.2 - with deprecation warning) and a 1.16.2-lts that you will need to explicitly whitelist like I did for this PR. I'm not aware of any way to allow to whitelist multiple pre-release tags spawning between different versions in a single command.

Keep in mind the proposed semver range still allows any newer stable release (1.16.2, 1.16.3, etc), so it's not going to affect consumers, except for the fact they will again see the deprecation warning if they opt-out from the lts

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

Maybe we could use the lts tag instead? This should work in theory assuming you always keep the lts tag present.

I played with it with https://semver.npmjs.com/ and using lts seems to work.

@FezVrasta
Copy link
Contributor Author

So it would be popper.js@lts? How would you define a specific version range though?

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

AFAICT you don't. The tag will always point to the latest lts tag. Similarly to what the latest tag does.

But anyway, that's something you should decide, I'm just spitballing here 🙂

So, basically this PR just needs the NuGet files updated and the rest should be fine. I'll review it after that locally.

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Apr 6, 2020

-      <dependency id="popper.js" version="[1.16.0,2)" />
+      <dependency id="popper.js" version="[1.16.1-lts,2)" />

Is this change correct? I have no idea what the ,2 refers to 😅

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2020

It basically means < 2 AFAICT. It's a range.

@FezVrasta
Copy link
Contributor Author

AFAICT you don't. The tag will always point to the latest lts tag. Similarly to what the latest tag does.

The 1.16.1-lts is already published on @lts so I think the command should work, in case a user runs npm i popper.js@lts though, the package.json will be populated with ^1.16.1.-lts, which allows 1.16.2 (non-lts), which may lead to some confusion if a new release is ever published.

@XhmikosR
Copy link
Member

XhmikosR commented Jul 3, 2020

@FezVrasta sorry for the late response here.

So, this PR seems to be ready as a first patch, but I was thinking maybe we should update to 1.16.1-lts in our devDependencies in another PR? There haven't been too many code changes AFAICT, just a small cleanup.

Also, is the project name "Popper" or "Popper.js"? We seem to be using both on our docs and I'd like to streamline this.

@XhmikosR
Copy link
Member

XhmikosR commented Jul 3, 2020

BTW I reverted the _config.yml change because the hashes won't match, hence why my point above about updating to 1.16.1-lts in our devDependencies.

@supergibbs
Copy link
Contributor

supergibbs commented Jul 3, 2020

Sorry, not sure how I missed this thread. For semver, 1.16.1-lts means prerelease and if bootstrap depends on it, it’ll be also be considered prerelease (by Nuget). You can use a +lts instead. Meant for build number or meta data and is ignored. So 1.16.1+lts would be the same as 1.16.1. Not sure how that would work since there is a 1.16.1 already. From what I can tell, there isn't any significant differences in 1.16.1 vs 1.16.1-lts so I vote updating Nuget to 1.16.1 or release a 1.16.1+lts and see what happens.

@FezVrasta FezVrasta closed this Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants