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

invalid import statement in Example #250

Closed
Jxck opened this issue Oct 5, 2015 · 4 comments
Closed

invalid import statement in Example #250

Jxck opened this issue Oct 5, 2015 · 4 comments

Comments

@Jxck
Copy link
Contributor

Jxck commented Oct 5, 2015

https://github.com/openpeer/ortc/blob/master/ortc.html#L1036

this import is invalid. and also helper js invalid for export, even in ES6 modules.

but, in my opinion, helper is not good for Example.
each Example should complete itself for easy to understand.
and this helper seems not useful enough to add to Draft.

for example

function trace(text) {
  // This function is used for logging.
  text = text.trimRight();
  if (window.performance) {
    var now = (window.performance.now() / 1000).toFixed(3);
    console.log(now + ": " + text);
  } else {
    console.log(text);
  }
}

we don't always need to calculate performance time.
it's enough to use console.trace or console.log

function errorHandler(error) {
  trace("Error encountered: " + error.name);
}

enough to use console.error

function mySendLocalCandidate(candidate, component, kind, parameters) {
  // Set default values
  kind = kind || "all";
  component = component || RTCIceComponent.RTP;
  parameters = parameters || null;

  // Signal the local candidate
  mySignaller.mySendLocalCandidate({
    "candidate": candidate,
    "component": component,
    "kind": kind,
    "parameters": parameters
  });
}

this used in some line, but mySignaller.mySendLocalCandidate also called directory too.
this means call mySignaller.mySendLocalCandidate directory is enough, and helper is removable.

  • myIceGathererStateChange
  • myIceTransportStateChange
  • myDtlsTransportStateChange

only a message helper.
just use console.log and write a description of that event in draft(already done) is enough.

wrap up.
I think remove helper and write more understandable and complete in each section is better for draft Example.
if you agree with this, I can PR for Fix all Examples.

or if you think this helper is necessary, I think it OK but please fix import / export syntax with ES6 valid one.

thanks.

@robin-raymond
Copy link
Contributor

@Jxck Here's my proposal. We are going to check in the latest version so that all fixes are up to date. Then I plan to run through a beautifier after (suggestions as to your favourite online one?) Then you can issue pull request against the sample. You seem to have a better handle on these than us :)

@aboba aboba added the 1.1 label Oct 6, 2015
aboba added a commit that referenced this issue Jan 6, 2016
@aboba aboba added the PR exists label Jan 6, 2016
aboba added a commit that referenced this issue Jan 7, 2016
Re-based fix for Issue #250
This was referenced Jan 7, 2016
@aboba
Copy link
Contributor

aboba commented Jan 15, 2016

Can someone who understands JS import/export take a look at the examples to see if they are now correct?

@Jxck
Copy link
Contributor Author

Jxck commented Jan 15, 2016

syntax seems correct to me.

@Jxck Jxck closed this as completed Jan 15, 2016
@Jxck
Copy link
Contributor Author

Jxck commented Jan 15, 2016

sorry I didn't aim to close. but if it's time to close. please close it.

@Jxck Jxck reopened this Jan 15, 2016
@aboba aboba closed this as completed Jan 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants