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

[1.8.2-beta.12] `import` statements break IE11 - screen stays blank #10658

Closed
smeijer opened this issue Jul 31, 2019 · 39 comments
Closed

[1.8.2-beta.12] `import` statements break IE11 - screen stays blank #10658

smeijer opened this issue Jul 31, 2019 · 39 comments
Assignees
Milestone

Comments

@smeijer
Copy link

@smeijer smeijer commented Jul 31, 2019

Version 1.8.2-beta.16, the most recent beta at this moment, is unable to run in IE.

The starter repo does work. But as soon as I add my first import, everything turns white. All other browsers that I can test (Chrome, Firefox, Edge) are working just fine.

The steps I took to reproduce the issue:

meteor create ie-import-issue --release 1.8.2-beta.16 --react
cd ie-import-issue
meteor npm install ramda
meteor

At this point, the application starts and is fully functional in IE 11

Now, go ahead and add the following import to /imports/ui/App.jsx (or any other client side js file)

import { compose } from 'ramda';

You'll see IE turn blank. Comment that line again, and IE functionality is restored.

I've pushed the repo to github: https://github.com/smeijer/meteor-182-ie-imports with the breaking change isolated in a dedicated commit: smeijer/meteor-182-ie-imports@0e88a10

meteor-issue

@jamesmillerburgess

This comment has been minimized.

Copy link
Contributor

@jamesmillerburgess jamesmillerburgess commented Aug 4, 2019

What is the latest version of meteor that doesn't break?

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Aug 5, 2019

Meteor 1.8.2-beta.11 is the latest version of meteor that doesn't break. Upgrade beta.11 to beta.12 and the imports are broken.

@smeijer smeijer changed the title [1.8.2-beta.16] `import` statements break IE11 - screen stays blank [1.8.2-beta.12] `import` statements break IE11 - screen stays blank Aug 5, 2019
@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 13, 2019

Hi any more on this please... in a world of hurt with IE11, yet again?

1.8.2-beta.11 I am getting Unable to get property <SomePackageName> of undefined or null reference` where is a variable I have exported in a package.js in a Meteor package I created. All works fine on non IE11 browsers (ES6 friendly).

@arggh

This comment has been minimized.

Copy link
Contributor

@arggh arggh commented Aug 13, 2019

@adamgins Perhaps try one of the newer betas? The latest is 1.8.2-beta.17.

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Aug 13, 2019

The import issue also exists in .17

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 13, 2019

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 15, 2019

@arggh @smeijer @benjamn this one is killing me... for the life of me I can't seem to get my app to build for IE11... I have tried .17, .11 back to 1.8.0.2 and 1.8.1 all borked and get various ES6 related errors. Any words of wisdom, besides don't support IE11 you can share please?

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Aug 15, 2019

I'm in the same boat @adamgins. We can't upgrade to 1.8.2 if IE11 isn't supported. Whether we like it or not, IE11 will be around for a long time.

There is a difference between not optimizing, or just showing a blank screen.

So, I guess we'll need to wait for a fix. I'm available for testing potential fixes though.

But, 1.8.1 should just work. So you might want to create a new ticket for that and tell us a bit more about the issues you're experiencing.

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 15, 2019

@smeijer yep, at least another 6 years... UGH!!!

@smeijer what version do you have working, I can't even get it to work with 1.8.0.2, 1.8.1 :-(

I get similar issues 1.8.1 see https://forums.meteor.com/t/error-meteorinstall-on-ie11/27173

In the past some how I tricked it into running and ended having to including my whole "node_modules" dir in my repo and that's what I deployed... so I did have a 1.8.1 with copied node_modules working but in recent builds something broke this for me. In short it's not doing the ecmascript things and coverting ES6 packages to ES5 which causes all sorts of issues.

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 15, 2019

Did you guys try instructuring meteor to transpile this node_module package?

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 15, 2019

I put a symbolic link to node_modules in my imports folder. Is that what you mean?

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 15, 2019

Yeah, I know Ben recently added another way to do that via a config file, but I'm assume the older way still works.

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 15, 2019

sorry, do you have a link to the new way please?

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 15, 2019

I'm trying to find it but I assume the older way still works.

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 15, 2019

Ok this is the PR I was referring to, it might be helpful.

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 16, 2019

@aliogaili so my initial test was to just add the entire node_modules folder to the imports with a symbolic link, do you know if this actually works or if I have to add symbolic links to each individual package?

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 16, 2019

It's not symbolic link, it's a configuration object, please take a look at the PR. Also you need to isolate the package that is causing the issue.

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 16, 2019

@aliogaili sorry I was referring to the "old" symbolic link way.

I am about to try with the new settings as per the PR

BTW, do you recommend the .17 beta or another one... seen a few other issues that reference other beta releases?

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 16, 2019

With beta .15, I get some other issues as per https://forums.meteor.com/t/error-meteorinstall-on-ie11/27173/6

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 16, 2019

I think you should use the latest, there were few 1.8 betas (beta 8 all the way to 15) where Meteor was aggressively compiling everything in node_modules. Latest only compile the packages in that configuration (and I assume the symbolic links are backward compatible, but I'm not sure that's what you need to double check).

However I think Meteor 1.8.2 betas are using benjamn's reify to compile the import statements in node_modules as well, which is a change from previous versions if I'm not mistaken, so this could be causing some issues since it's a new change.

But I'm sure Ben will figure it out.

PS IE is a nightmare, and it's only 7% of the market share (mostly legacy enterprises using it), I highly encourage taking every opportunity to push the client to upgrade when possible, read this blog.

@benjamn benjamn self-assigned this Aug 16, 2019
@benjamn benjamn added this to the Release 1.8.2 milestone Aug 16, 2019
@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 17, 2019

I think I found the culprit iron router...

I created a base app meteor create... and added in all my packages and NPM modules and it seems like iron:router causes it to come up with the meteorInstall issue on IE11 - similar to https://forums.meteor.com/t/error-meteorinstall-on-ie11/27173

ugh I have been delaying moving to flow-router, lots of dependencies.

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Aug 18, 2019

Ok this is the PR I was referring to, it might be helpful.

I see benjamn self-assigned this issue, so there is hope that it will be fixed (soon) 😃. I hope that "fix" you (@aliogaili) mention, will not be the way to solve this (original import issue), as recompiling all node_modules would increase build time. And it would be a breaking change. You can check out my repo (https://github.com/smeijer/meteor-182-ie-imports), and see the import breaks when upgrading beta 11 to beta 12. It's not that it's just a single node module. It's pretty much all of them.

Which would result in two issues.

  1. the list under nodeModules.recompile would become quite long.
  2. and more importantly, in a larger application, it's a hell to debug white screens. And figure out which import creates this and which to add to recompile.

PS IE is a nightmare, and it's only 7% of the market share (mostly legacy enterprises using it), I highly encourage taking every opportunity to push the client to upgrade when possible,

I agree. I don't understand that Microsoft has build worlds most advanced IDE, but somehow was never able to provide proper debugging tools for IE. IE11 isn't even that bad. But the lack of dev tools, is a hell.

Any way, 7% is globally. "only 7%", is still to much to ignore. And as you write, "mostly legacy enterprises using it". Unfortunately, mostly users at large organizations use it. Even though 100% of our users do have either Chrome, or Edge installed. Still close to 40% use IE 11. Our users are non-technical users at larger corporate and governmental organizations. They see the IE icon, and don't bother trying Edge instead.

I agree; we should try to push them. Show a "browser upgrade banner" or something like that. But for now, it's to early to kill support entirely. What is what this issue is doing.

@adamgins, I think your issues are different, and not related to the import statements. The error you mention Unable to get property <SomePackageName> of undefined or null reference, is a generic one and usually caused by non supported syntax. It's always caused by a crash upon page load (blank screen).

Try something like forcing the legacy bundle. And trace down when the page crash by excluding parts from your bundle. Render a blank layout on purpose. Start with your root components, and slowly including back other parts of the page.

import { setMinimumBrowserVersions } from 'meteor/modern-browsers';

setMinimumBrowserVersions({
  ie: Infinity,
});

But, this is all off topic. I want to suggest to return this issue to it's original scope. import statements break IE. This to help benjamn by keeping issues clean and tidy.

@aliogaili

This comment has been minimized.

Copy link

@aliogaili aliogaili commented Aug 19, 2019

As recompiling all node_modules would increase build time.

@smeijer yes, you're right. It did increase the build time significantly but this behaviour was reverted in beta-15 upward that is why I recommend testing from this version.

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 24, 2019

@aliogaili @smeijer @benjamn I think I found my issue... circular imports... #10679

@adamgins

This comment has been minimized.

Copy link

@adamgins adamgins commented Aug 26, 2019

@aliogaili @smeijer @benjamn in addition to the curcular issue above I found the the JSONATA package was causing an issue too.. they do have jsonata-es5.js not sure how to get it to pick that up when on IE11?

Update: got 'jsonata` to work with a symbolic link

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

Trying to get this thing working in IE again, And I've come a big way with recompiling modules. Unfortunately, IE is currently crashing on @babel/runtime, and it doesn't seem that I can force that one to be recompiled.

Anyone else experiencing this?

image

{
  "meteor": {
    "mainModule": {
      "client": "./src/client/main.js",
      "server": "./src/server/main.js"
    },
    "nodeModules": {
      "recompile": {
        "@babel/runtime": "legacy",
        "@emotion": "legacy",
        "@emotion/is-prop-valid": "legacy",
        "@emotion/memoize": "legacy",
        "create-emotion": "legacy",
        "create-emotion-styled": "legacy",
        "rc-tooltip": "legacy",
        "react-ga": "legacy",
        "react-intl": "legacy",
        "whatwg-fetch": "legacy"
      }
    }
  }
}
@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

Is there an easy way to add all es6 modules to that recompile list? For every dep I'm currently:

  • inspecting the reported breaking line
  • identifying the library
  • adding that to the recompile list
  • stop meteor server (ctrl + c)
  • start meteor server
  • F5 IE
  • open dev tools
  • F5 IE
  • inspecting the reported breaking line ....

I'm starting to have a huge list, but it feels like a never ending process. As I also need to add child dependencies. For example, the breaking dep: node_modules/@apollo/react-common/node_modules/tslib/tslib.es6.js. I need to add tslib, as adding @apollo/react-common doesn't fix the issue. But just before tslib, I needed to add ts-invariant, which is also required by @apollo/react-common.

Can't we just transpile all modules for the legacy bundle instead?

@benjamn

This comment has been minimized.

Copy link
Member

@benjamn benjamn commented Nov 6, 2019

@smeijer The general reason we can't recompile @babel/runtime/helpers/* is that some of the helpers use the very syntax they are implementing/extending. For example, the helper that provides a version of typeof that can return "symbol" uses the native typeof syntax for the other cases, so compiling that module results in a broken typeof polyfill. However, your screenshot suggests that Meteor's compilation of import and export syntax (specifically, the module.export(...) bit) is slightly broken in this case, which is something I think I can fix.

@benjamn

This comment has been minimized.

Copy link
Member

@benjamn benjamn commented Nov 6, 2019

After further investigation, I see that @babel/runtime/helpers/esm/extends.js is being properly compiled without arrow functions for the legacy bundle:

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//                                                                                                                     //
// node_modules/@babel/runtime/helpers/esm/extends.js                                                                  //
//                                                                                                                     //
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                                                                                                                       //
module.export({default:function(){return _extends}});function _extends() {
  _extends = Object.assign || function (target) {
    for (var i = 1; i < arguments.length; i++) {
      var source = arguments[i];

      for (var key in source) {
        if (Object.prototype.hasOwnProperty.call(source, key)) {
          target[key] = source[key];
        }
      }
    }

    return target;
  };

  return _extends.apply(this, arguments);
}

Here's how I temporarily forced Chrome to receive the legacy bundle:

// In server/main.js:
import { setMinimumBrowserVersions } from "meteor/modern-browsers";
setMinimumBrowserVersions({
  chrome: Infinity,
});

While code generated by Meteor will never depend on @babel/runtime/helpers/esm/* in the first place, there are some npm packages that do, which is why we compile import and export in those modules (but nothing else).

Unless I'm missing something, I think this is all working as designed?

@benjamn

This comment has been minimized.

Copy link
Member

@benjamn benjamn commented Nov 6, 2019

From my investigations, a blanket policy of recompiling everything for the legacy bundle not only worsens build performance, but also produces incorrect code in a number of npm packages that were not intended to be recompiled. More so than the performance concerns, that's why this process has to remain selective, and we will never be able to blindly recompile every package.

In other words, as far as I can see, the solution to this issue is to use the new meteor.nodeModules.recompile configuration, even though it can be somewhat tedious.

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

@benjamn , thanks for the research. I understand the conclusion. At the same time, it also worries me a bit. This app worked fine before. So if this stays the standard, upgrade will be a pita for users that need to support IE.

In the mean while, I'll continue the debug loop, and post back the result that I'm dealing with. I believe that the most frustrating is, that I also need to specify deep dependencies. Resulting in the situation that I don't know when I'm done.

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

@benjamn, how about recompiling the child dependencies, if we specify a direct dependency? Just now for example, I caught zen-observable-ts which is required by apollo-link-ws. Can't we make it so that declaring apollo-link-ws: legacy would also recompile it's dependencies?

That way, I at least know that the number of libraries to inspect is limited.

* edit; regarding

and we will never be able to blindly recompile every package

How did beta.11 do this? As that version worked without the recompile list. #10658 (comment)

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

I'm starting to see a pattern here. All modules that are breaking IE, do have a module field in their package.json that's being used. Can't we use the main field for the legacy bundle?

I'm at breaking module number 67 now. I really feel like I'm missing something here. As more and more packages will start add the module field, more packages will break IE support.

image

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 6, 2019

#10658 (comment)

That is without declaring at in the recompile list?

I also tried to force Chrome to use the legacy bundle now, just to see if I had the same results. But indeed, an arrow function. Something is off here. Can this have anything to do with configuration in tsconfig or babelrc?

image

@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 7, 2019

After adding 128 modules to the recompile list, I gave up on that approach. This can't be how it's supposed to be.

My findings are that it's #10541 that broke my app in IE.

When I prioritize main over module, IE works again. With 0 modules declared in the recompile list.

Again, I don't have any troubles with defining my own, direct dependencies. But the requirement of also needing to chase down all deep dependencies, really sucks. Especially because IE isn't known for it's debugger, and every time recompile receives a new dependency, a full restart is required (ctrl+c -> meteor). And Windows is still not as performant as Linux/Mac, so it all adds up.

Also, this would mean that with every single dependency update, the witch hunt would start all over, while having no idea how many iterations it will take this time.

So, at first, I fixed it by fixing the build tool a bit on my side:

this.mainFields = ["browser", "module", "main"];

- this.mainFields = ["browser", "module", "main"]; 
+ this.mainFields = ["browser", "main", "module"]; 

mainFields: ["browser", "module", "main"],

- mainFields: ["browser", "module", "main"], 
+ mainFields: ["browser", "main", "module"], 

tryMain("browser");
tryMain("module");
tryMain("main");

 tryMain("browser"); 
+ tryMain("main"); 
 tryMain("module"); 
- tryMain("main");

That turned out to work just fine. Needless to say, this would break as I would upgrade or reinstall Meteor. So my second solution was to write a postinstall script that runs after npm install (add to package.json » scripts » "postinstall"

const fs = require('fs');

const getModules = projectDir =>
  fs
    .readdirSync(`${projectDir}/node_modules`)
    .map(ns => {
      if (ns[0] === '@') {
        return fs
          .readdirSync(`${projectDir}/node_modules/${ns}`)
          .map(name => `${projectDir}/node_modules/${ns}/${name}/package.json`);
      }

      return `${projectDir}/node_modules/${ns}/package.json`;
    })
    .reduce(
      (acc, path) => (Array.isArray(path) ? [...acc, ...path] : [...acc, path]),
      [],
    )
    .filter(path => fs.existsSync(path))
    .map(path => ({
      pkg: JSON.parse(fs.readFileSync(path, { encoding: 'utf8' })),
      path,
    }));

const fixPackages = projectDir => {
  const modules = getModules(projectDir);
  let count = 0;

  modules.forEach(({ path, pkg }) => {
    if (pkg.module && pkg.main && !pkg.browser) {
      pkg.browser = pkg.main;
      fs.writeFileSync(path, JSON.stringify(pkg, '', '  '), {
        encoding: 'utf8',
      });
      count++;
    }
  });

  console.log(`modified ${count} packages to prefer 'main' over 'module'`);
};

fixPackages('./');

As Meteor prefers browser over module over main, I now run this script on postinstall. It rewrites all package.jsons in node_modules that don't have a browser field, by setting browser to become main.

Effectively, preferring main over module again. As I populate the browser field, and not just prioritizing the main field, I'm sure that this doesn't affect node modules.

The script is also available as gist: https://gist.github.com/smeijer/d205b9a86537fbad6186781423165d5c

@benjamn, this works. But I still believe this should be fixed in the builder.

@benjamn

This comment has been minimized.

Copy link
Member

@benjamn benjamn commented Nov 8, 2019

Can't we use the main field for the legacy bundle?

@smeijer I think that's a fantastic idea! Let me give that a try in the next RC.

benjamn added a commit that referenced this issue Nov 8, 2019
#10658 (comment)
#10658 (comment)

As usual, changes to module resolution logic need to happen in parallel in
tools/isobuild/resolver.ts and in packages/modules-runtime. However,
thanks to the modern/legacy system, it's easy to make the modules-runtime
package behave exactly the way(s) we want in the server, modern client,
and legacy client bundles.
@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 8, 2019

Awesome! Can't wait to try it out 🎉

@benjamn

This comment has been minimized.

Copy link
Member

@benjamn benjamn commented Nov 8, 2019

Ok! After you run meteor update --release 1.8.2-rc.7, the legacy bundle will be built using the "main" field rather than the "module" field. Since this was the behavior for all bundles in Meteor 1.8.1, I expect this change should reduce the amount of meteor.nodeModules.recompile configuration that's necessary when updating to Meteor 1.8.2.

Thanks again to @smeijer for raising concerns about the amount of work involved, and to everyone else for your patience while we iron out these last issues!

@benjamn benjamn closed this Nov 8, 2019
@smeijer

This comment has been minimized.

Copy link
Author

@smeijer smeijer commented Nov 10, 2019

Thanks @benjamn, fix works as expected! With rc.7, my app doesn't require any of the dependencies to be added in the recompile list. Just like before.

Your work is much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.