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

minification of three.dart project breaks it #24694

Closed
Tomucha opened this issue Oct 22, 2015 · 6 comments
Closed

minification of three.dart project breaks it #24694

Tomucha opened this issue Oct 22, 2015 · 6 comments

Comments

@Tomucha
Copy link

Tomucha commented Oct 22, 2015

I'm attaching simple three.dart demo, which gets broken by "pub build", but it works after "pub build --mode=something".

How to reproduce:
Download: https://dl.dropboxusercontent.com/u/6822278/bugreport-minify-three-js.zip

Run:
pub get
pub build --mode=devel (to omit minification)

Then open build/web/index.html you should se "3D forest". So far so good.

Now run:
pub build

Try to open build/web/index.html - you will see error in Chrome console:
Uncaught TypeError: Cannot read property 'toString' of null

Expected behavior:
The result should be the same, only after "pub build" JS should be smaller.

Conslusion:
There might me something wrong with three.dart library, however this is very unexpected behavior. Minification shouldn't influence final JS.

Version:
pub --version
Pub 1.12.1

@floitschG
Copy link
Contributor

I looked at this. This is not a problem with the minification, but with the order of evaluation.
In the release-version, the html-file executes the JavaScript before the html-page is completely created. Therefore the call to document.querySelector("#webgl") returns undefined which leads to the null-pointer exception that you are seeing.
If I replace the index.html in the release build with the one from the debug build, everything works.

I'm not sure what the correct approach is, but I'm ccing a few people that know more about this.
/cc @sigmundch @vsmenon

Minor side note: please include the pubspec.lock the next time to make sure we are testing with the same version.

@vsmenon
Copy link
Member

vsmenon commented Oct 22, 2015

You should be able to fix this by moving your script tag to the very end of the body of the html file (just before the and after the element with id="webgl"). That would be the most portable solution across multiple browsers.

Basically, the release version is starting more quickly than the debug one. If the script code is at the bottom of the page, you shouldn't notice a semantic difference.

@Tomucha
Copy link
Author

Tomucha commented Oct 23, 2015

Thanks for superquick response, clearly you are right, works for me now. I'll contact the author of the original three.dart example I started from and let him know.

However this kind of inconsistent behavior is very confusing ... is there anything what can be done?

  • some kind of warning when accessing DOM before it's fully loaded?
  • more strict evaluation in Dartium (it worked in Dartium too)
  • some super-smart transformer / best practice enforcer for Pub?

I can fill out a feature request somewhere if you want (and if you see any possible solution).

Thanks!

@vsmenon
Copy link
Member

vsmenon commented Oct 23, 2015

The release behavior is consistent with the HTML5 spec - which requires scripts to be executed synchronously (unless marked defer or async) as the page is parsed. In some cases, folks want to access the DOM early for perf / page load reasons.

The bug here is the Dartium lazy behavior - but it will be very difficult to fix (Dartium processes import and part directives via async requests - in dart2js, it's all compiled into one file and easier to load).

A lint / transformer sounds like a good idea. I understand that the Dart-Polymer transformer is already moving scripts to the end. @sigmundch @pq @kevmoo might have some ideas here.

@sigmundch
Copy link
Member

  • some super-smart transformer / best practice enforcer for Pub?

I believe you are using it :-), it just needs a bug fix.

If I understand correctly, it appears that the problem actually originated from the dart_to_js_script_rewritter transformer you use in your pubspec. The transformer replaces the Dart + browser/dart.js script tags, by a tag that just loads the .js code. But it only does so when you use --mode=release (that's why you didn't see the problem in debug mode). The default way you had written the original code was actually OK and didn't have issues because the browser/dart.js script ensures that we don't load the .js code until after DOMContentLoaded.

The fix would be to improve the rewriter transformer to add a defer keyword to the script tag. That would guarantee that the timing is correct after removing the browser/dart.js file.

I filed Workiva/dart_to_js_script_rewriter#21 in the rewriter repo to track this. /cc @sethladd who I believe maintains the rewriter.

@sigmundch
Copy link
Member

Closing this issue as this is tracked now by the issue in dart_to_js_script_rewriter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants