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.

Sign up for GitHub

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

[#4429] Fix Error Retrieving Old Config Options #4451

Merged
merged 1 commit into from
Oct 6, 2020
Merged

[#4429] Fix Error Retrieving Old Config Options #4451

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,12 @@
this.handleReqFailure(e);
return;
}
const reMajorVersion = /v(\d+)/;

const excludedTagVersions = new Set(['v0.7', 'v0.8.1']);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the update, this is indeed a much simpler diff 😆 I don't recall off hand what the browser compatibility breakdown is for Set's, although for that matter I don't know if there's any potential compatibility issues elsewhere within this page.

Do you know what the compatibility story is for Set.has? I suppose we could fallback to an array with an includes or indexOf if necessary, or even and object with key checks.

These are both really small data collections so the browser compatibility weighs more heavily than performance and/or ergonomic gains of newer constructs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the MDN compatibility chart for determining if there were any issues with utilizing the set and set.has functionality: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

It seemed pretty widely adopted so I went with it, but I'm totally open with using the array functions if we wanted to go that route. I guess like, what were we hoping to achieve with compatibility with the page? What are the thresholds we are shooting for with browser versions getting supported?

Copy link
Member

Choose a reason for hiding this comment

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

I guess like, what were we hoping to achieve with compatibility with the page? What are the thresholds we are shooting for with browser versions getting supported?

Great question, and to be perfectly honest it's completely undefined to the best of my knowledge. The fact that this will even work on IE (albeit 11) is more than good enough for me.


const tagOptions = tags
.map(tag => tag.name)
.filter(tag => tag.startsWith('v'));
.filter(tag => tag.startsWith('v') && !excludedTagVersions.has(tag));
this.versionOptions = this.versionOptions.concat(tagOptions);
},
updated() {
Expand Down