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

Added missing browser mapping and default fallback #272

Merged
merged 2 commits into from
Dec 29, 2019

Conversation

pwfcurry
Copy link
Contributor

Previously unknown browsers would cause failures with unhelpful messages, e.g.,

<feature> is not supported in undefined 2.5  compat/compat

I've added a mapping for KaiOS & a fallback to raw name when no mapping is available.

Copy link
Owner

@amilajack amilajack left a comment

Choose a reason for hiding this comment

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

Thanks for this addition! After adding some tests to this PR, we can merge this. Also please fix the linter errors with yarn lint --fix

@pwfcurry
Copy link
Contributor Author

pwfcurry commented Nov 12, 2019

Thanks for this addition! After adding some tests to this PR, we can merge this. Also please fix the linter errors with yarn lint --fix

I've fixed the lint error. Due to the external dependency on browserlist I can't see a sensible way to test the fallback behaviour (I've now fixed the specific issue I was having, with kaios), other than exposing the formatTargetNames and unit testing that method only. Suggestions welcome!

@amilajack
Copy link
Owner

Try something like this in Versioning.spec.js:

it('should support string config in rule option', () => {
  const config = DetermineTargetsFromConfig('some-non-existent-browser-version');
  const result = Versioning(config);
  expect(result).toEqual({
    // ...
  });
});

@pwfcurry
Copy link
Contributor Author

pwfcurry commented Nov 12, 2019

This will fail because the browserlist dependency doesn't know about "some-non-existent-browser".

This is a non issue if the list of browsers & mappings within this project is always in parity with browserlist. Perhaps there's a better solution to this.

@amilajack
Copy link
Owner

I think a better solution to this would be using caniuse-db as a peerDependency. @ljharb thoughts on this?

@ljharb
Copy link
Collaborator

ljharb commented Nov 13, 2019

It’d only need to be a peer if it need to be a singleton - peer deps are required, not optional.

@amilajack amilajack merged commit 23266f9 into amilajack:master Dec 29, 2019
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

3 participants