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

[BUG] Fix introduced in #60 breaks backwards compatibility with old node.js versions #65

Closed
pniewiejski opened this issue Feb 28, 2020 · 4 comments
Assignees
Labels

Comments

@pniewiejski
Copy link

@pniewiejski pniewiejski commented Feb 28, 2020

What / Why

Changes introduced in #60 (specifically in commit: f2cdfcf) break compatibility for older versions of node.js which do not support const & let variable declarations. 

Notice that in v2.8.7, apart from one place in tests, it is the only place where const (or let for that matter) is used in the whole project.

So before #60 this project was backwards compatible and after #60 it is not. I think this should be considered a breaking change.

This fix (#60) was introduced between v2.8.6 and v2.8.7. Introducing breaking changes and bumping only the patch number is IMHO a practice that should be avoided.

Where / affected versions

Affected version is v2.8.7.
node.js version: 0.10.* (Yes, it's still there... 😢)

Expected outcome

Change const declaration to var.

// index.js +123
const authmatch = giturl.match(/[^@]+@[^:/]+/)
@ljharb
Copy link

@ljharb ljharb commented Feb 29, 2020

This is breaking me as well for a number of packages https://travis-ci.com/inspect-js/has-template-literals/jobs/292483820

v2 can't use const or let, in order to maintain compatibility. Can v2.8.7 be reverted, or updated to only use var?

ljharb added a commit to inspect-js/has-template-literals that referenced this issue Feb 29, 2020
@darcyclarke
Copy link
Member

@darcyclarke darcyclarke commented Feb 29, 2020

@ljharb @pniewiejski we'll revert this to use var & ship a patch either before or on this next Tuesday

@darcyclarke darcyclarke added this to the OSS - Sprint 5 milestone Feb 29, 2020
@darcyclarke darcyclarke self-assigned this Feb 29, 2020
@ljharb
Copy link

@ljharb ljharb commented Feb 29, 2020

Thanks! It’d be great to do it sooner, since all my packages’ CI is failing in the meantime unless i add a commit pegging the version of hosted-git-info. Perhaps you could make “latest” be 2.8.6 right now, so that people don’t have to get 2.8.7 unless they ask for it?

darcyclarke added a commit that referenced this issue Feb 29, 2020
…ssed node 6 support
darcyclarke added a commit that referenced this issue Feb 29, 2020
…ssed node 6 support
darcyclarke added a commit that referenced this issue Feb 29, 2020
…ssed node 6 support

PR-URL: #66
Credit: @darcyclarke
Close: #66
Reviewed-by: @darcyclarke
@darcyclarke
Copy link
Member

@darcyclarke darcyclarke commented Feb 29, 2020

This should be fixed by #66 which was landed in hosted-git-info@2.8.8. We'll try to roll out a cli update as soon as possible which will include this dep update (likely as npm@6.14.2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants