Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compress_attributes browserify transform and fixup unexpected characters in bundles #5426

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 20, 2021

Fixes #5382.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jan 20, 2021
@archmoj archmoj added this to the NEXT milestone Jan 20, 2021
@archmoj
Copy link
Contributor Author

archmoj commented Jan 20, 2021

At first the publish test passed. Now it fails.

@archmoj
Copy link
Contributor Author

archmoj commented Jan 20, 2021

Publish tests pass on d3-geo pull #5112 after merging master.
So maybe that's what we should consider?

@nicolaskruchten
Copy link
Contributor

what's the relationship between the d3-geo PR and these weird characters?

@alexcjohnson
Copy link
Collaborator

@nicolaskruchten d3's geo functions are where these weird characters come from, it uses greek letters in its source code for angles in the projections. d3v4 converts all its source code to ASCII:

To the consternation of some users, 3.x employed Unicode variable names such as λ, φ, τ and π for a concise representation of mathematical operations. A downside of this approach was that a SyntaxError would occur if you loaded the non-minified D3 using ISO-8859-1 instead of UTF-8. 3.x also used Unicode string literals, such as the SI-prefix µ for 1e-6. 4.0 uses only ASCII variable names and ASCII string literals (see rollup-plugin-ascii), avoiding encoding problems.

It seems like this PR, while a very good hypothesis, doesn't solve whatever the underlying problem is. Let me take a look at #5112 again, that seems to me a more promising approach to get rid of this randomly-recurring issue permanently.

@archmoj
Copy link
Contributor Author

archmoj commented Jan 20, 2021

Thanks @alexcjohnson for reviewing #5112.
I refactored the compress task in ed0195b so that debugging would be easier now & in future.
With that on a dev branch I also tried single replacements and other encoding options. Despite various attempts, no solution is found there.
In the end in order to complete this PR, I reverted the changes here f68214b.

Returning Plotly object from register_extra function seems like a good idea to me (unless we hit this problem again).
Obviously we could revert that commit again once we were able to get #5112 in.

To conclude this PR

  • let no-bad-char test catch the error instead of no-new-func test;
  • refactored and slightly improved tasks/compress_attributes.js;
  • fixed the issue on master by changing the way how we make and export the Plotly object.

@archmoj archmoj changed the title Fixup compress_attributes browserify transform to avoid unexpected characters in bundles Improve compress_attributes browserify transform and fixup unexpected characters in bundles Jan 20, 2021
@archmoj archmoj requested a review from alexcjohnson January 20, 2021 23:41
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

OK, we can do it this way for now if it lets us avoid the error.

FWIW I think the reason the module.exports = require('./register_extra')(Plotly); bothers me is that it makes it unclear what exactly you're exporting. A specific consequence is it may be unclear to people making their own bundles what to do if they don't want these extras. But anyway I suspect we'll be toggling that commit until we get a real solution 😅
💃

@archmoj archmoj merged commit 1e8f1b8 into master Jan 21, 2021
@archmoj archmoj deleted the fixup-compress-attributes branch January 21, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected character in some bundles
3 participants