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

add console.log #39

Closed
wants to merge 0 commits into from
Closed

add console.log #39

wants to merge 0 commits into from

Conversation

moldray
Copy link

@moldray moldray commented Dec 28, 2017

No description provided.

@therustmonk
Copy link
Member

Thank you for the PR! 👍
Could you add console_warn (console.warn) to this service too?
And could you rebase it to master to avoid redundant merging?

@moldray
Copy link
Author

moldray commented Dec 28, 2017

@deniskolodin , Thanks for your reply!
I know javascript well, but I am a beginner of Rust. Give me some time. Do be patient!

@rivertam
Copy link
Contributor

All other console methods would be cool too. 🙂

@RangerMauve
Copy link

I think this contribution would be better if it was sent to stdweb, that way other projects using Rust to WASM will have console support out of the box, too.

@therustmonk
Copy link
Member

Console service is useful for us, and I think it's enough to keep only log and warn methods in this PR. Later we can add more methods by other PRs to prevent accumulation of long-living PRs.

Also I will create issue in stdweb to discuss the implementation of console bridge in stdweb crate. We could use it later when it will be implemented.

@rivertam
Copy link
Contributor

rivertam commented Dec 28, 2017

@deniskolodin what's the purpose of splitting the different console methods into different PRs? They're pretty much identical and parallel.

I'm not referring to things like console.table, but console.error, console.count, etc. Things that only accept simple strings or no arguments.

@rivertam
Copy link
Contributor

On another note: it would be cool to see an overload of the logging methods that do get implemented taking a vector of strings to pass as args to the logging methods. Might be overkill though.

@therustmonk
Copy link
Member

@rivertam There is no special reason. I didn't want to make this PR too complicated, because @moldray said he is starting to use Rust. But if you write the list of methods you want I think @moldray could add more. Which methods do you think will be useful from Console API for this framework?

@rivertam
Copy link
Contributor

rivertam commented Dec 28, 2017

I mean, ideally the entire console API is available through the service, no?

I'd say there's a pecking order in terms of how hard it would be to implement various methods.

Super easy

These can just be copied and pasted from console_log as it is now and replace log with something else

  • console_warn
  • console_error
  • console_info
  • console_debug
  • console_count (with argument)
  • console_time (with argument)
  • console_time_end (with argument)
  • console_profile (with argument)
  • console_profile_end (with argument)

Really easy

These just have no arguments

  • console_clear
  • console_count
  • console_group
  • console_group_collapsed
  • console_group_end
  • console_profile
  • console_profile_end
  • console_timestamp
  • console_trace

Medium

  • console_assert (takes a bool and a string)

Hard

  • console_dir
  • console_dir_xml
  • console_table

It looks like a lot, but the entire first section is basically just a few copy-pastes, the second section is even simpler, and console_assert should be almost as simple. I agree the hard section could definitely go in a separate PR. Another thing that could go in this or another PR would be checking to make sure the non-standards (profile, profile_end, timestamp, dir, and dir_xml) exist before calling them.

I wouldn't mind tacking these on myself at all, but this PR already exists. Kind of seems silly to me to not add them now. I'd understand more if we were only doing log for now, but doing just log and warn seems a bit arbitrary.

@wldcordeiro
Copy link
Contributor

@rivertam I'd go over the methods and see what's most commonly used too I don't recall the console API having the best spec so some methods are present on only some browsers.

@wldcordeiro
Copy link
Contributor

It'd be nice to update the examples to use this service as well since currently they use println! and that just gets routed to console.log

@rivertam
Copy link
Contributor

rivertam commented Dec 28, 2017

@wldcordeiro I'd argue looking at commonness makes sense iff it were hard to implement these functions. I referenced Mozilla's documentation and reference the fact that some of them are non-standard (and I could see an argument for not including the non-standards, though they're pretty cool and I don't see much harm in implementing them anyways)

@wldcordeiro
Copy link
Contributor

wldcordeiro commented Dec 28, 2017

Also one thing you mentioned that most can be done with copy/paste but to me that means you could abstract them in some way and simplify it further. Most are very similar but really smaller incremental PRs will win over larger monolithic ones.

@rivertam Why don't you copy/paste your comment above as an issue?

@rivertam rivertam mentioned this pull request Dec 28, 2017
22 tasks
@rivertam
Copy link
Contributor

#44

I agree that there should be a way to abstract them, though I'm not sure how we'd do it in a typesafe way. I know in C++ we could basically just use a macro like CONSOLE_METHOD(profile_end, profileEnd), but I'm not sure how direct text substitution translates to Rust (not familiar with Rust macros, really)

@moldray
Copy link
Author

moldray commented Dec 29, 2017

In my opinion, log and error is necessary. log is most usefull in develop, and sometimes you need to throw error. I write js many years, but I never used such as infodebugcount etc. I dont't think we need implement these in yew.

before coding, we'd better have more discussion. and then, i will have more time to study rust and improve my code 😄

at last, happy new year!!

新年快乐!

@moldray
Copy link
Author

moldray commented Dec 29, 2017

As I used a wrong way to update my master branch, some rebase difficulty came to me, so I create a new PR #48

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

5 participants