[DebuggerV2] Add shim for loadMonaco() method & supporting 3rd-party libraries #3411
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephanwlee
approved these changes
Mar 23, 2020
Contributor
stephanwlee
left a comment
stephanwlee
left a comment
There was a problem hiding this comment.
Your internal only BUILD file may need some content.
Contributor
Author
|
Thanks for the review, @stephanwlee . I added a comment to the internal BUILD file so that it is non-empty. |
bileschi
pushed a commit
to bileschi/tensorboard
that referenced
this pull request
Apr 14, 2020
…libraries (tensorflow#3411) * Motivation for features / changes * Roll forward go/tbpr/3374 with fixes. * Technical description of changes * Same as go/tbpr/3374, with the following exception: the `tf_web_library` BUILD targets for requirejs, monaco-editor (core) and monaco-languages are moved to a dedicated BUILD file (tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/monaco/BUILD) to facilitate sync'ing. * Detailed steps to verify changes work correctly (as executed by you) * copybara change CL: CL/302218915 * Final state: CL/302222581. Build passes, after some manual reverts deal with JSCompiler artifacts * Alternate designs / implementations considered * Alternative: use copybara rules to make the requirejs, monaco-editor and monaco-languages rules BUILD internally. * Con: Those transformed targets are *not* used internally. This is potentially confusing to future developers. It is also an unnecessary maintenance overhead. * Con: While using `tensorboard_html_binary` to wrap around third_party libraries may work for requirejs, which involves a single .js file, it is more tedious for monaco-editors and monaco-languages, which involve multiple separate files (5 for monaco-editors and 3 for monaco-languages). This would involve a large number of confusing rules added to the copybara file. Those rules would be useless except for making the build pass (see the previous Con item). Some of these files are not .js or .css files (e.g., the .ttf file), further increasing the complexity.
bileschi
pushed a commit
that referenced
this pull request
Apr 15, 2020
…libraries (#3411) * Motivation for features / changes * Roll forward go/tbpr/3374 with fixes. * Technical description of changes * Same as go/tbpr/3374, with the following exception: the `tf_web_library` BUILD targets for requirejs, monaco-editor (core) and monaco-languages are moved to a dedicated BUILD file (tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/monaco/BUILD) to facilitate sync'ing. * Detailed steps to verify changes work correctly (as executed by you) * copybara change CL: CL/302218915 * Final state: CL/302222581. Build passes, after some manual reverts deal with JSCompiler artifacts * Alternate designs / implementations considered * Alternative: use copybara rules to make the requirejs, monaco-editor and monaco-languages rules BUILD internally. * Con: Those transformed targets are *not* used internally. This is potentially confusing to future developers. It is also an unnecessary maintenance overhead. * Con: While using `tensorboard_html_binary` to wrap around third_party libraries may work for requirejs, which involves a single .js file, it is more tedious for monaco-editors and monaco-languages, which involve multiple separate files (5 for monaco-editors and 3 for monaco-languages). This would involve a large number of confusing rules added to the copybara file. Those rules would be useless except for making the build pass (see the previous Con item). Some of these files are not .js or .css files (e.g., the .ttf file), further increasing the complexity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tf_web_libraryBUILD targets for requirejs, monaco-editor (core) and monaco-languages are moved to a dedicated BUILD file (tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/monaco/BUILD) to facilitate sync'ing.tensorboard_html_binaryto wrap around third_party libraries may work for requirejs, which involves a single .js file, it is more tedious for monaco-editors and monaco-languages, which involve multiple separate files (5 for monaco-editors and 3 for monaco-languages). This would involve a large number of confusing rules added to the copybara file. Those rules would be useless except for making the build pass (see the previous Con item). Some of these files are not .js or .css files (e.g., the .ttf file), further increasing the complexity.