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

Console and Error Report files modularized #14466

Merged
merged 3 commits into from
Jul 27, 2018

Conversation

Piyush3079
Copy link
Contributor

@Piyush3079 Piyush3079 commented Jul 8, 2018

This pr contains the PMA Console and PMA Error Report code in the modular form. The old files were split into multiple files to separate out functions required for base functionality.
There is some issue with this pr.
1). Earlier all the global functions were wrapped in the context of ErrorReport but now due to js modules it will not be possible to wrap all the imports in the context of ErrorReport.
2). Cross-origin files are now not allowed in Tracekit so stacktrace cannot be generated in developer mode.
3). The stack trace generated will contain a single line with column number because of uglified js file as output. See Tracekit Issue: occ/TraceKit#61

This pr is in continuations of Mod_Js_Server_Privileges. Before reviewing this pr, review Mod_Js_Server_Privileges and merge and then compare with this pr.

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@codecov
Copy link

codecov bot commented Jul 25, 2018

Codecov Report

Merging #14466 into gsoc-js-refactoring will decrease coverage by 0.65%.
The diff coverage is 0.67%.

@@                    Coverage Diff                    @@
##             gsoc-js-refactoring   #14466      +/-   ##
=========================================================
- Coverage                  50.53%   49.88%   -0.66%     
  Complexity                 14434    14434              
=========================================================
  Files                        501      504       +3     
  Lines                      68006    68895     +889     
=========================================================
  Hits                       34369    34369              
- Misses                     33637    34526     +889

@devenbansod devenbansod changed the base branch from master to gsoc-js-refactoring July 26, 2018 14:21
@devenbansod devenbansod self-assigned this Jul 26, 2018
@devenbansod devenbansod self-requested a review July 26, 2018 14:22
@Piyush3079 Piyush3079 force-pushed the Mod_Js_Console branch 4 times, most recently from e186ba5 to 77b9d15 Compare July 26, 2018 14:47
Copy link
Member

@devenbansod devenbansod left a comment

Choose a reason for hiding this comment

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

Looks good, though the files and functions might require renaming later.

I'll test locally once you've confirmed it works on your end.

Copy link
Contributor Author

@Piyush3079 Piyush3079 left a comment

Choose a reason for hiding this comment

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

General comments to increase understandability of this pr

* This function needs to be changes as no objects are available
* in the global scope so window does not contain any function
*/
wrap_global_functions: function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functions are now not available in global scope, therefore need to find some alternate way to wrap exported functions in ErrorReport object and call all the functions in the context of ErrorReport.

@@ -0,0 +1,338 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is not in working state completely because of some issues with webpack output files.

  1. The output is the uglified file, therefore the stack trace is not that clear as it was earlier.
  2. Due to cross-origin file loading in developer mode, context will not be available in developer mode.

Copy link
Member

Choose a reason for hiding this comment

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

Do you foresee this working in near future? can something like sourcemaps or something be used to improve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, source maps can help but there is already an issue on tracekit occ/TraceKit#61 for adding support of source maps. Also, source maps increase the size of output to almost 2.5 times.
I am still looking for ways to deal with this.

@Piyush3079 Piyush3079 force-pushed the Mod_Js_Console branch 4 times, most recently from b949ce8 to 6aeb531 Compare July 27, 2018 14:25
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
TODO: To find a way to wrap the exported functions containing PMA_ prefix into the error handler to generate backtrace.
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
@Piyush3079 Piyush3079 force-pushed the Mod_Js_Console branch 2 times, most recently from 2df0f07 to de78e79 Compare July 27, 2018 15:23
Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
@devenbansod
Copy link
Member

Everything except the selenium job is passing at https://travis-ci.org/phpmyadmin/phpmyadmin/builds/408533738.

Merging this to unblock #14467

@devenbansod devenbansod merged commit ff81454 into phpmyadmin:gsoc-js-refactoring Jul 27, 2018
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

2 participants