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

FastBoot.require is broken when running locally #221

Closed
bobisjan opened this issue May 12, 2019 · 7 comments · Fixed by #227
Closed

FastBoot.require is broken when running locally #221

bobisjan opened this issue May 12, 2019 · 7 comments · Fixed by #227

Comments

@bobisjan
Copy link
Contributor

bobisjan commented May 12, 2019

The following error occurs with ember-fetch installed:

Error: Cannot find module 'abortcontroller-polyfill/dist/cjs-ponyfill' from '/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees'
    at Function.module.exports [as sync] (/Users/bobisjan/Developer/fastboot-require-bug/node_modules/resolve/lib/sync.js:58:15)
    at Object.require (/Users/bobisjan/Developer/fastboot-require-bug/node_modules/fastboot/src/ember-app.js:139:42)
    at Module.callback (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/ember-fetch/fetch-fastboot.js:6:42)
    at Module.exports (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:106:1)
    at requireModule (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:27:1)
    at r (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:176:1)
    at Module.callback (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/addon-tree-output/ember-data/-private.js:14244:1)
    at Module.exports (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:106:1)
    at Module._reify (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:143:1)
    at Module.reify (/var/folders/m3/2d92q9yx06j2q51zh2_w8js80000gn/T/broccoli-76884UZpwDvEhdhpO/out-206-broccoli_merge_trees/assets/vendor/loader/loader.js:130:1)

The problem is that resolve.sync can not find module based on the basedir set to distPath https://github.com/ember-fastboot/fastboot/blob/master/src/ember-app.js#L139.

This does not occur in previous version (1.x), because of this fallback https://github.com/ember-fastboot/fastboot/blob/master/src/ember-app.js#L151, I assume that this works only by accident, because that branch was meant (based on the comment) only for Node.js internal modules like path, os, ...

The "production" usage with fastboot-app-server is not affected, because it requires to run npm install in "dist" directory (manually http://ember-fastboot.com/docs/deploying#deploying-your-app or automatically eq. when using S3 downloader https://github.com/ember-fastboot/fastboot-s3-downloader/blob/master/index.js#L110), hence the resolve.sync works fine.

Possible quick fixes

Remove the basedir option from resolve.sync call.

or

Run npm install inside distPath after build from ember-cli-fastboot add-on.

Proper fix

Deprecate FastBoot.require for importing external modules (only internals are allowed like path, os, ...) and use ember-auto-import add-on.

What do you think? @kratiahuja, @rwjblue, @xg-wang

@xg-wang
Copy link
Member

xg-wang commented May 13, 2019

Isn't external module allowed to be required per #200? Can you expand on how ember-auto-import should used by Fastboot users?

Setting basedir to distPath seems correct if fastboot expects node_modules structure in /dist #198 (comment). Maybe the fix should be linking node_modules to dist in ember-cli-fastboot serve.

@bobisjan
Copy link
Contributor Author

Isn't external module allowed to be required per #200?

Yes, fix in that PR was about external modules for production like environment (fastboot-app-server). This is working in our app on AWS, and it was working locally too.

The error mentioned here showed up when upgrading to Ember CLI 3.5+, which now uses system tmp - this "disconnects" local project's node_modules from distPath (when I disable SYSTEM_TMP experiment the problem goes away, but I assume that this is not a problem of the experiment).

Setting basedir to distPath seems correct if fastboot expects node_modules structure in /dist #198 (comment).

Agree, with ember-auto-import the basedir: distPath can go away completely.

Maybe the fix should be linking node_modules to dist in ember-cli-fastboot serve.

That's IMHO a possible quick fix too.

Can you expand on how ember-auto-import should used by Fastboot users?

This comes from our experience with the ember-auto-import (so it may not be 💯 correct 🙈). The add-on allows to specify dependency in package.json and "automagically" import it like you would normally do import foo from 'bar'; (skipping app.import with custom shim), it also allows to dynamically ("lazy") import stuff, eq. we use this for loading specific modules per Node.js and browser like:

if (typeof FastBoot !== undefined) {
  return import('ldclient-node');
} else {
  return import('ldclient-js');
}

@xg-wang
Copy link
Member

xg-wang commented May 15, 2019

Yea seems it works with ember-cli-fastboot because it falls back to requireing built-in modules - but it's actually making require to pwd node_modules.
image

@bobisjan Is it failing for you locally when developing with fastboot-app-server?
Not sure should Fastboot go with linking node_modules or making ember-auto-import default.

Seems ember-auto-import bundles everything into a fastboot file rather than doing require. Does it work with the case code inside auto-import-fastboot.js still using Fastboot.require?

Also curious @ef4's opinion on this.

@bobisjan
Copy link
Contributor Author

Is it failing for you locally when developing with fastboot-app-server?

No, it's failing with ember s.

I'm not sure about linking ${project}/node_modules - with ember s this means to make link from system tmp. What happens to symlinks when system decides to clear tmp?

@xg-wang
Copy link
Member

xg-wang commented May 15, 2019

@bobisjan what is your ember-data version?
This works for me 👉 xg-wang/__fastboottemp@c4b42f9

@bobisjan
Copy link
Contributor Author

@xg-wang, ember-data@3.9.3.

The example you've linked is using fastboot@1.2.1, the resolve.sync is from fastboot@2.0.0 (I've a forked ember-cli-fastboot with fastboot@2.0.1 to test).

@bobisjan
Copy link
Contributor Author

This is zonkyio/ember-cli-fastboot@0853d8f how I temporarily solved the issue by installing FastBoot dependencies in distPath.

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

Successfully merging a pull request may close this issue.

2 participants