This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105

Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom
0.15-issue104-source-map
Dec 7, 2018
Merged

Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom
0.15-issue104-source-map

Conversation

@Marc-Andre-Rivet

@Marc-Andre-RivetMarc-Andre-Rivet commented Dec 6, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#104
Relates to plotly/dash#478
Relates to plotly/dash-table#284
Relates to plotly/dash-core-components#404

  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package

Marc-Andre-Rivet
Marc-Andre-Rivet commented Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Adding map files, reordering

Marc-Andre-Rivet
Marc-Andre-Rivet commented Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py Outdated
'https://unpkg.com/dash-renderer@{}'
'/dash_renderer/dash_renderer.min.js.map'
).format(__version__),
'namespace': 'dash_renderer'

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Add maps to Dash

@T4rk1nT4rk1nDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)

@T4rk1nT4rk1nDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So it seems this additional entry is required

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file

@T4rk1nT4rk1nDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.

Marc-Andre-Rivet
Marc-Andre-Rivet commented Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Bump version

Marc-Andre-Rivet
Marc-Andre-Rivet commented Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
"name": "dash-renderer",
"version": "0.15.0",
"version": "0.15.2",

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM

Marc-Andre-Rivet
Marc-Andre-Rivet commented Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
library: dashLibraryName,
libraryTarget: 'window',
},
devtool: 'source-map',

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/

@T4rk1nT4rk1nDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?

@T4rk1nT4rk1nDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.

@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:
mode === 'development' ? 'eval-source-map' : 'source-map'
which will give you the old behavior in dev

valentijnnieman
valentijnnieman approved these changes Dec 6, 2018
View reviewed changes

@valentijnniemanvalentijnnieman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.

valentijnnieman
valentijnnieman approved these changes Dec 6, 2018•
edited
Loading
View reviewed changes
@valentijnnieman

valentijnnieman commented Dec 6, 2018

Copy link
Copy Markdown
Contributor

Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!

@Marc-Andre-Rivet

Marc-Andre-Rivet commented Dec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff

@T4rk1n

T4rk1n commented Dec 6, 2018

Copy link
Copy Markdown
Contributor

Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.

@Marc-Andre-Rivet

Marc-Andre-Rivet commented Dec 6, 2018

Copy link
Copy Markdown
ContributorAuthor

That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.

@T4rk1n

T4rk1n commented Dec 6, 2018

Copy link
Copy Markdown
Contributor

The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.

💃

@Marc-Andre-Rivet

Marc-Andre-Rivet commented Dec 7, 2018

Copy link
Copy Markdown
ContributorAuthor

The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.

Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.

@Marc-Andre-RivetMarc-Andre-Rivet mentioned this pull request Dec 7, 2018
Merged
@Marc-Andre-Rivet
Marc-Andre-Rivet merged commit 4c324db into masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 104 - Add source map to NPM and PyPi packages - #105
Merged
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Dec 7, 2018
Merged
Issue 104 - Add source map to NPM and PyPi packages#105
Marc-Andre-Rivet merged 4 commits into
masterfrom 
0.15-issue104-source-map
Conversation
@Marc-Andre-Rivet
@Marc-Andre-RivetMarc-Andre-Rivet
commented
Dec 6, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#104

Relates to plotly/dash#478

Relates to plotly/dash-table#284

Relates to plotly/dash-core-components#404
  • Add source-map to build output
  • Add source-map to npm package
  • Add source-map to PyPi package
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadMANIFEST.in
include dash_renderer/dash_renderer.dev.js
include dash_renderer/dash_renderer.dev.js.map
include dash_renderer/dash_renderer.min.js
include dash_renderer/dash_renderer.min.js.map
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Adding map files, reordering
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/__init__.py
Outdated

 'https://unpkg.com/dash-renderer@{}'
 '/dash_renderer/dash_renderer.min.js.map'
 ).format(__version__),
 'namespace': 'dash_renderer'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Add maps to Dash
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Not sure it needs an entry here for the map, the _js_dist are files automatically served on the index. I think the js bundle will request the map on it's own.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

You're right that the JS file will request it on its own if the dev tools are opened -- but it will resolve it to its relative path, for example
image
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Or will Dash still allow the request to go through even if it's not defined here?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In any case, I'll try it -- I assumed it was necessary :)
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes dash should allow it, before the map changes in dash I remember seeing 500 errors for map requests.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Tested it out against the dash-table's examples and if the map is not present here, it doesn't get loaded. Dash probably tells the browser there is no such resource; and the browser continues on with its life?
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So it seems this additional entry is required
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes, but the 500 only happens if there is no extension support for map and the map is in the init file
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah your right just looked at the code and we check the if the path is registered so it needs to be in the dist for dash to allow it.
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threaddash_renderer/version.py
@@ -1 +1 @@
__version__ = '0.15.1'
__version__ = '0.15.2'
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadpackage.json
{
 "name": "dash-renderer",
 "version": "0.15.0",
 "version": "0.15.2",
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Bump version.. for some reason this had not been bumped to 0.15.1 for NPM
Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018
View reviewed changes
Comment threadwebpack.config.js
 library: dashLibraryName,
 libraryTarget: 'window',
 },
 devtool: 'source-map',
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Source map! This setting is slower but more accurate than other mappings. See here: https://webpack.js.org/configuration/devtool/
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Need to be sure I don't lose my debugging support with this, I remember the plain source-maps did not work so well.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Is this the in-IDE support you mentioned to me a little while ago?
@T4rk1nT4rk1nDec 6, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes, I'll try it to be sure.
@Marc-Andre-RivetMarc-Andre-RivetDec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thanks. If it doesn't work I'll change it to:

mode === 'development' ? 'eval-source-map' : 'source-map'

which will give you the old behavior in dev
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018
View reviewed changes
@valentijnniemanvalentijnnieman
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

🎉 Love it! Been wanting to get source maps in for a while but never got around to it. Approving it right away but let's make sure @T4rk1n's IDE support doesn't break.
valentijnnieman
valentijnnieman
approved these changes
Dec 6, 2018•

edited

Loading
View reviewed changes
@valentijnnieman
valentijnnieman
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Got a 500 error the first time I did a review, seems it posted it twice now. Whoops!
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
If anyone wonders, the dev js file is smaller but removing the "eval source map" causes it to take 1 line per code line instead of squishing everything (with evals), explaining the 30k lines diff
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
Getting a Uncaught SyntaxError: Unexpected token : log with the dev bundle.
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 6, 2018

Copy link


Copy Markdown

ContributorAuthor
That's because we're not using the dynamic flag yet. When loaded "correctly" through the devtools this will not happen anymore.
@T4rk1n
T4rk1n
commented
Dec 6, 2018

Copy link


Copy Markdown

Contributor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
💃
@Marc-Andre-Rivet
Marc-Andre-Rivet
commented
Dec 7, 2018

Copy link


Copy Markdown

ContributorAuthor
The external code map works for debugging, looked like it was even more stable for dev bundle. The min bundle also debugged but it was jumping over things.
Thanks for checking this out. I thought we might be able to get rid of the dev build after the change but if you see improved behavior when debugging with it, let's keep it.
@Marc-Andre-RivetMarc-Andre-Rivet
mentioned this pull request
Dec 7, 2018
 Merged

@Marc-Andre-Rivet
Marc-Andre-Rivet
merged commit 4c324db
into
masterDec 7, 2018
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@valentijnniemanvalentijnniemanvalentijnnieman approved these changes
@T4rk1nT4rk1nT4rk1n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@Marc-Andre-Rivet@valentijnnieman@T4rk1n