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

Fail when unknown field exists on share() and canShare()? #214

Closed
saschanaz opened this issue Aug 27, 2021 · 14 comments
Closed

Fail when unknown field exists on share() and canShare()? #214

saschanaz opened this issue Aug 27, 2021 · 14 comments
Labels
revisit Stuff we should revisit in future revisions of the spec

Comments

@saschanaz
Copy link
Member

(#211 (comment))

navigator.share({
  text: "text",
  unknown: "unknown",
});

The current behavior ignoring unknown silently is not very useful nor intuitive. I suspect that changing the behavior won't break existing uses and will allow partial implementations (e.g. Firefox as of 2021) fail early without being hacky.

@saschanaz
Copy link
Member Author

saschanaz commented Aug 27, 2021

Furthermore, the current difference between files unsupported at browser level and at OS level is confusing.

navigaor.share({
  text: "text",
  
  // this will be silently ignored when unsupported at browser level
  // but will explicitly fail when unsupported at OS level
  files: [new File()],
});

I think it should either be ignored in both cases or fail in both cases.

@marcoscaceres
Copy link
Member

I guess this really depends on the meaning of "can share", right? The spec, and by general convention with WebIDL APIs ignoring unknown members, seems to suggest that so long as one or more members matches, then something is sharable.

That solution, which maybe we should add as an example to the spec, is to do something like the following:

const data = { /* a bunch of members, some supported, some not... */ };
// Check them individually
const canShareEvery = Object.entries(data).every(([k, v]) => navigator.canShare({[k]: v}));

Or some variant of the above... like, one could filter on the unsharable members and remove those things from the application.

const sharableMembers = Object.entries(data).filter(([k, v]) => navigator.canShare({[k]: v}));

Or one could write a simple "yep/nope" splitter.

@saschanaz
Copy link
Member Author

Maybe deprecate the multi member situation in that case?

@marcoscaceres
Copy link
Member

marcoscaceres commented May 30, 2022

Well, that gets us back to the original proposal where you could only test if a member name was supported. I'm inclined to cut our loses here because (although clunky), a developer can use the code above to check exactly what is supported.

If it turns out .canShare() can't be used correctly, then we should revisit some alternative API..

(as an aside, if we find generally in the platform that we need a "only these members" IDL check, we might want to push that up to WebIDL itself as some extended attribute... something like):

[ThrowOnUnknownMember]
dictionary {
 }

If we accept the current design, I then just realized this is a duplicate of #211

@saschanaz
Copy link
Member Author

saschanaz commented May 30, 2022

a developer can

We know this "can" does not mean they will 😄. The current API form strongly suggests doing bad thing, so I doubt anyone will go the repeated-single-member without explicit deprecation of the multi member thing.

@marcoscaceres
Copy link
Member

marcoscaceres commented May 30, 2022

I certainly don't disagree (gets us back to #108).

The question is really if it's enough of a problem for us to do anything about at this point?

My feeling is no (given lack of people coming here to complain... but not a great data point). I'd be ok to just live with .canShare() as is and move onto other things.

@saschanaz
Copy link
Member Author

The thing is, this will become worse and worse if the spec "moves on other things" and add features.

I'm okay to keep it as is if nobody intends to add anything else here, but if someone ever does then we need to talk about this.

@marcoscaceres
Copy link
Member

agree. But given we haven't added any new features is last 2 years, and the feature is more or less working as expected, I feel pretty comfortable with us moving onto other things.

@saschanaz
Copy link
Member Author

Not sure what you mean by moving onto other things here. I'd argue that some note should there so that any future editors won't accidentally add anything.

@marcoscaceres
Copy link
Member

Not sure what you mean by moving onto other things here.

I mean investing time on other specs and/or standardizing other parts of the web platform. I'd like for this spec to be "done" (i.e., moved to CR) so I can focus on other specs (e.g., Permissions, Web Manifest, etc.). This spec takes up quite a bit of time, but it's not, IMO, has high priority as some other specs I could be spending time on.

I'd argue that some note should there so that any future editors won't accidentally add anything.

The Editors are not resigning or anything. I don't think that's a risk.

@saschanaz
Copy link
Member Author

saschanaz commented Jun 6, 2022

I mean investing time on other specs and/or standardizing other parts of the web platform. I'd like for this spec to be "done" (i.e., moved to CR) so I can focus on other specs (e.g., Permissions, Web Manifest, etc.). This spec takes up quite a bit of time, but it's not, IMO, has high priority as some other specs I could be spending time on.

canShare() has been modified multiple times since the initial issue and there's a pending PR from you, so I doubt such thing won't happen without some explicit note, a pinned issue, or anything that remind future people (and ourselves in the future) that it's faulty. (That said, this note thing is indeed #211)

Edit: See also whatwg/webidl#107 for the IDL issue.
Edit: Anyway I don't have a big interest to fix anything other than the YouTube issue, so maybe just add Revisit tag and do the tag says.

@marcoscaceres marcoscaceres added the revisit Stuff we should revisit in future revisions of the spec label Jun 6, 2022
@marcoscaceres
Copy link
Member

Created a Revisit label + added.

@marcoscaceres
Copy link
Member

Actually, can we close this one as it's the same as #108?

@saschanaz
Copy link
Member Author

#108 is too long with no conclusion, this one at least has a concrete suggestion 🤔

But this didn't attract any interest here, so let's go back to #108.

@saschanaz saschanaz closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
revisit Stuff we should revisit in future revisions of the spec
Projects
None yet
Development

No branches or pull requests

2 participants