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

1.6.1-rc.1: cordova app contains only `[object Object]` in index.html #9521

Closed
macrozone opened this Issue Jan 9, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@macrozone

macrozone commented Jan 9, 2018

I updated to 1.6.1-rc.1 from 1.6.0 and my cordova build does not work anymore on IOS (did not test with android)

the app does not start. Using safari as inspector i see that the index.html only contains the string [object Object]

The app runs correctly as a webpage.

EDIT: this happens on newly created project as well, I tried also previous beta version. The last working version was meteor 1.6.0.1!

So... REPRODUCTION:

  • meteor create myapp
  • cd myapp
  • meteor add-platform ios
  • meteor update --release 1.6.1-rc.1
  • meteor run ios

@macrozone macrozone changed the title from 1.6.1-rc.1: cordova app only contains `[object Object]` in index to 1.6.1-rc.1: cordova app only contains `[object Object]` in index.html Jan 9, 2018

@macrozone macrozone changed the title from 1.6.1-rc.1: cordova app only contains `[object Object]` in index.html to 1.6.1-rc.1: cordova app contains only `[object Object]` in index.html Jan 9, 2018

@macrozone

This comment has been minimized.

macrozone commented Jan 9, 2018

ok, i can rule out the babelrc file, as empty file leads to the same problem

I also cleaned .meteor/local, reinstalled meteor, node_modules, etc.: same result

@macrozone

This comment has been minimized.

macrozone commented Jan 9, 2018

I am currently testing it with checked out version of meteor 1.6.1.x, same result...

What i found so far:

  • in tools/cordova/builder.js there is this line:
  files.writeFile(files.pathJoin(applicationPath, 'index.html'),
      bootstrapPage, 'utf8');

where bootstrapPage is a stream. calling toString() on that object returns [object object] wheras doing something like

var streamToString = function(stream, callback) {
        var str = '';
        stream.on('data', function(chunk) {
          str += chunk;
        });
        stream.on('end', function() {
          callback(str);
        });
      }
    streamToString(bootstrapPage, function(myStr) {
      console.log("bootstrap", myStr);
    });

will log the correct string,

so i assume that files.writeFile no longer treats streams correctly

@macrozone

This comment has been minimized.

macrozone commented Jan 9, 2018

ok, it's the changes in a5866e2#diff-5ce4ea994a8e901a5beb2a3a777682bc

it seems this change has not been made to the cordova build.

I am astonished that such a serious bug does make it into a release-candidate... no one uses cordova with meteor anymore?

EDIT:

i am now running the self-test "cordova builds with server" --slow on devel and expect it to fail. So this bug should have been noticed. Is there a way to see failing tests on PRs and release-candidates?

benjamn added a commit that referenced this issue Jan 9, 2018

Rename Boilerplate#toHTML to toHTMLStream and deprecate toHTML.
PR #9343 changed the return type of Boilerplate#toHTML from String to
Stream, which is likely to break existing code that expects a string.

In order to make the change in return type more obvious, I have renamed
the method to toHTMLStream, and I have attempted to update all call sites
appropriately. However, because this change comes in the release candidate
phase of Meteor 1.6.1 testing, it seemed important to preserve the
string-returning behavior of toHTML, with a deprecation notice.

Unless third-party code is using the Boilerplate class directly, I don't
think the toHTML method will ever be called, and we can remove it in
Meteor 1.6.2.

Thanks to @macrozone for tracking this problem down.

Fixes #9521.
@benjamn

This comment has been minimized.

Member

benjamn commented Jan 9, 2018

@macrozone You're a step ahead of me, but the reason that test wasn't running is that we skip "slow" tests for PR builds, for no good reason except to provide feedback faster. I'm going to remove the "slow" marker from that test so that it runs regularly, since clearly it's an important test.

benjamn added a commit that referenced this issue Jan 9, 2018

Reenable cordova-builds self-test, which would have caught #9521.
Self-tests that are marked as "slow" are not run automatically for pull
requests, but they should be run before a release.

It was a mistake to publish the first Meteor 1.6.1 release candidate
without running these "slow" tests, as the cordova-builds test would have
caught the problem reported by @macrozone in #9521.

I've decided to remove the "slow" marker from this test, since it's
important for checking Cordova sanity, and clearly could fail.
@benjamn

This comment has been minimized.

Member

benjamn commented Jan 10, 2018

This should be fixed if you run meteor update --release 1.6.1-rc.2. Still working on a proper regression test, though.

@macrozone

This comment has been minimized.

macrozone commented Jan 10, 2018

@benjamn maybe you should run all tests at least for rc-builds ;-) but thank you, at least i had a chance to do a do a my first pr against meteor ;-)

@macrozone macrozone closed this Jan 10, 2018

benjamn added a commit that referenced this issue Jan 11, 2018

Reenable cordova-builds self-test, which would have caught #9521.
Self-tests that are marked as "slow" are not run automatically for pull
requests, but they should be run before a release.

It was a mistake to publish the first Meteor 1.6.1 release candidate
without running these "slow" tests, as the cordova-builds test would have
caught the problem reported by @macrozone in #9521.

I've decided to remove the "slow" marker from this test, since it's
important for checking Cordova sanity, and clearly could fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment