Make sure lazy engines with async routes are handled #150

jackson-dean · 2019-09-17T22:30:40Z

I was running into this error when loading a lazy engine route:

TypeError: route.prefetch is not a function

route in this case was actually a promise which resolved to a route with prefetch method.

This converts the change set in the current iteration to a promise to ensure the correct fullParams are used and then normalizes the route to always be a promise so they are treated the same for both cases.

jackson-dean · 2019-09-17T22:38:10Z

There is a failing test which I will look into if this is generally the right thing to do.

jackson-dean · 2019-09-19T02:42:47Z

It looks like all tests pass but there is some weird issue where running yarn does not generate node_modules for the linked lazy-engine. https://travis-ci.org/nickiaconis/ember-prefetch/jobs/586819795

I also see this issue locally. however if I simply run yarn again everything works out fine. any ideas where I might be going wrong?

jackson-dean · 2019-09-19T04:40:14Z

Ok, a little tinkering with the travis configs and I finally got everything working.

jackson-dean · 2019-09-20T05:15:36Z

after some extra experimenting I'm not so sure this the right thing to do. but at least it is here to get feedback and start a discussion about it.

rwjblue

Changes generally seem good to me, there are a few things to resolve (some have inline comments as well):

Split out into a couple separate PR's for:
- Dropping Node 6, 9, and 11 support
- Updating CI config to use yarn instead of npm
- The runtime functionality change (probably keep in this PR)
Confirm that CI is running against [email protected],3.8,3.12
Change the promise handling around a bit (using RSVP.Promise.resolve instead of RSVP.resolve)
Confirm that we do not expect to synchronously have a route._prefetch in the case of non-lazy engines. I suspect this is an assumption that we have, which this PR is invalidating. To address, we would need to set route._prefetched synchronously when the route wasn't a promise and set it async when it is.

.gitignore

.travis.yml

addon/services/prefetch.js

.gitignore

addon/services/prefetch.js

In preparation for adding ember-engines for testing lazy routes, this sets node version for CI to 8 and updates node engines to reflect that 8 is the lowest supported version. Also updates to use yarn for installing depdencies. The flags are added to prevent install failures related to having "link" dependencies in package.json. I referred to ember-engines repo for these changes. https://github.com/ember-engines/ember-engines/blob/v0.8.2/package.json#L43 https://github.com/ember-engines/ember-engines/blob/v0.8.2/.travis.yml#L54 Related PR: #150

rwjblue · 2019-09-20T19:51:26Z

Ready for a rebase now that #151 is landed.

jackson-dean · 2019-09-23T20:30:47Z

@rwjblue Thanks for all of your previous feedback! bump, whenever you get a chance. This is actually a blocker for us atm. Thank you!!

rwjblue · 2019-09-24T14:31:14Z

sorry for the delay, will try to re-review today...

jackson-dean · 2019-09-24T15:45:27Z

ok cool, I also noticed a bug as soon as I looked at the code again so I updated it and added an additional test to make sure prefetch actually works for a routeable lazy engine. however, now there is some funky eslint issue... looking into it

jackson-dean · 2019-09-24T15:55:00Z

it looks like yarn lint:js started failing after I added more files in the engine. it's not immediately obvious to me what is busted, but this is error I see when I run it locally:

Cannot find module '@ljharb/eslint-config'
Referenced from: /Users/jcdean/ws/ember-prefetch/tests/dummy/lib/lazy-engine/node_modules/broccoli-babel-transpiler/node_modules/resolve/.eslintrc
Error: Cannot find module '@ljharb/eslint-config'
Referenced from: /Users/jcdean/ws/ember-prefetch/tests/dummy/lib/lazy-engine/node_modules/broccoli-babel-transpiler/node_modules/resolve/.eslintrc
    at ModuleResolver.resolve (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:479:28)
    at load (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:551:26)
    at configExtends.reduceRight (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:425:36)
    at Array.reduceRight (<anonymous>)
    at applyExtends (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:403:26)
    at loadFromDisk (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:523:22)
    at Object.load (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config/config-file.js:559:20)
    at Config.getLocalConfigHierarchy (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config.js:227:44)
    at Config.getConfigHierarchy (/Users/jcdean/ws/ember-prefetch/node_modules/eslint/lib/config.js:179:43)

jackson-dean · 2019-09-24T19:25:22Z

however, I see a completely different error in the CI log:

ESLint: 4.19.1.
ESLint couldn't find the plugin "eslint-plugin-prettier". This can happen for a couple different reasons:
1. If ESLint is installed globally, then make sure eslint-plugin-prettier is also installed globally. A globally-installed ESLint cannot find a locally-installed plugin.
2. If ESLint is installed locally, then it's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
    npm i eslint-plugin-prettier@latest --save-dev
If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn lint:js" exited with 1.

jackson-dean · 2019-09-24T19:47:48Z

well, i came across this ember-cli/rfcs#121

jackson-dean · 2019-09-24T20:35:31Z

created a separate PR for updating the linting packages #154

jackson-dean · 2019-09-24T20:44:38Z

I ran eslint with these changes on top of the eslint changes and confirmed that it runs successfully locally.

Lazy engine routes are initially promises which resolve to route instances once the modules are loaded. In these cases prefetch current throws an error 'TypeError: route.prefetch is not a function' since it tries to call prefetch directly on the promise. It seems this used to be handled properly but was lost in a more recent refactor. This is just adding the functionality back. It's important to note that we do not preload the lazy engine assets in the test environment here because preloaded engine routes are not promises but the actual route instance. - Adds ember-engines to dev dependencies for testing. - Create a lazy engine in the dummy app. - Make sure we can visit this engine without preloading its assets.

addon/services/prefetch.js

rwjblue · 2019-11-19T16:35:38Z

Thank you for all the work here @jackson-dean! This was ultimately replaced by #159 which was released in v4.0.1.

Please, let me know if there are other changes/fixes that you are still needing...

jackson-dean · 2019-11-19T19:18:23Z

👍 I took a look. It does seem it will fix the issue of prefetch not getting called on a lazy engine, but it still seems we could end up passing unexpected transition and fullParams when the route resolves since those values are extracted in a loop outside of the promise callback. Whether it is actually a problem in practice I'm not sure. If not, it really does not matter that much.

xg-wang · 2019-11-19T23:54:58Z

when the route resolves since those values are extracted in a loop outside of the promise callback

Thanks @jackson-dean for pointing out! Is this the code you're referring to? https://github.com/tildeio/router.js/blob/v6.2.5/lib/router/route-info.ts#L240
This is a public API in ember https://api.emberjs.com/ember/3.13/classes/Route/methods/serialize?anchor=serialize.

However the serialization happens after waiting model hook this.getModel(transition). It does not make sense for prefetch hook to wait model hook.

I think it’s fine to not wait the serialized params, but a bit documentation about https://api.emberjs.com/ember/3.13/classes/Route/methods/serialize?anchor=serialize would not be called for the prefetch(params, transition) and the transition tenable isn’t fully resolved yet.

jackson-dean · 2019-11-20T15:10:40Z

Hey @xg-wang my concern was just misplaced. I was talking about here https://github.com/nickiaconis/ember-prefetch/pull/159/files#diff-54a7d68a8480ef36da94321be6541ca1R29 but my instinct was completely incorrect and just a consequence of working with js before let and const was even a thing 😅 , but just by virtue of using let which has proper scoping semantics, everything does indeed work out fine! If this were all var declarations it would be broken, but because js is more sane now it is perfectly fine 😄

However, if this code is transpiled with babel to old var statements, I think there still could be a problem 🤔 Unless babel is somehow smart enough to resolve this inconsistency.

jackson-dean · 2019-11-20T15:22:42Z

actually, on yet another look, it looks it will even be fine in any case because the outer promise immediately invokes its resolver with the outer route and fullParams. So, yes, at the end I don't have any concern about it. I had also initially missed this line https://github.com/nickiaconis/ember-prefetch/pull/159/files#diff-54a7d68a8480ef36da94321be6541ca1R23 which also mitigates any concern.

nice work, and thanks for fixing this! 😄

jackson-dean force-pushed the master branch 3 times, most recently from 8106dce to d3df4d5 Compare September 19, 2019 04:30

jackson-dean force-pushed the master branch 8 times, most recently from bfd6223 to ab03a39 Compare September 20, 2019 04:30

rwjblue requested changes Sep 20, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

addon/services/prefetch.js Outdated Show resolved Hide resolved

addon/services/prefetch.js Outdated Show resolved Hide resolved

jackson-dean force-pushed the master branch from ab03a39 to 92eef42 Compare September 20, 2019 14:44

jackson-dean commented Sep 20, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

jackson-dean force-pushed the master branch from 92eef42 to a86f092 Compare September 20, 2019 14:57

jackson-dean commented Sep 20, 2019

View reviewed changes

addon/services/prefetch.js Outdated Show resolved Hide resolved

jackson-dean force-pushed the master branch from a86f092 to 3721621 Compare September 20, 2019 15:30

jackson-dean mentioned this pull request Sep 20, 2019

Drop support for Node 6 and 11. #151

Merged

jackson-dean force-pushed the master branch 7 times, most recently from b054022 to 56eb602 Compare September 20, 2019 16:48

jackson-dean commented Sep 20, 2019

View reviewed changes

addon/services/prefetch.js Outdated Show resolved Hide resolved

jackson-dean force-pushed the master branch from 56eb602 to 097b088 Compare September 20, 2019 21:20

jackson-dean requested a review from rwjblue September 20, 2019 23:18

jackson-dean force-pushed the master branch 3 times, most recently from 1e20ee7 to c878262 Compare September 24, 2019 15:42

jackson-dean force-pushed the master branch from c878262 to 1d3e64d Compare October 1, 2019 15:22

jackson-dean commented Oct 5, 2019

View reviewed changes

addon/services/prefetch.js Show resolved Hide resolved

xg-wang mentioned this pull request Nov 15, 2019

Ensure prefetch hook will be called on routes within a lazy engine #159

Merged

rwjblue closed this Nov 19, 2019

nickiaconis mentioned this pull request Jan 22, 2020

Error while transitioning to in-repo-engines #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure lazy engines with async routes are handled #150

Make sure lazy engines with async routes are handled #150

jackson-dean commented Sep 17, 2019 •

edited

Loading

jackson-dean commented Sep 17, 2019

jackson-dean commented Sep 19, 2019 •

edited

Loading

jackson-dean commented Sep 19, 2019

jackson-dean commented Sep 20, 2019 •

edited

Loading

rwjblue left a comment

rwjblue commented Sep 20, 2019

jackson-dean commented Sep 23, 2019

rwjblue commented Sep 24, 2019

jackson-dean commented Sep 24, 2019 •

edited

Loading

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019 •

edited

Loading

rwjblue commented Nov 19, 2019

jackson-dean commented Nov 19, 2019

xg-wang commented Nov 19, 2019

jackson-dean commented Nov 20, 2019 •

edited

Loading

jackson-dean commented Nov 20, 2019 •

edited

Loading

Make sure lazy engines with async routes are handled #150

Make sure lazy engines with async routes are handled #150

Conversation

jackson-dean commented Sep 17, 2019 • edited Loading

jackson-dean commented Sep 17, 2019

jackson-dean commented Sep 19, 2019 • edited Loading

jackson-dean commented Sep 19, 2019

jackson-dean commented Sep 20, 2019 • edited Loading

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue commented Sep 20, 2019

jackson-dean commented Sep 23, 2019

rwjblue commented Sep 24, 2019

jackson-dean commented Sep 24, 2019 • edited Loading

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019

jackson-dean commented Sep 24, 2019 • edited Loading

rwjblue commented Nov 19, 2019

jackson-dean commented Nov 19, 2019

xg-wang commented Nov 19, 2019

jackson-dean commented Nov 20, 2019 • edited Loading

jackson-dean commented Nov 20, 2019 • edited Loading

jackson-dean commented Sep 17, 2019 •

edited

Loading

jackson-dean commented Sep 19, 2019 •

edited

Loading

jackson-dean commented Sep 20, 2019 •

edited

Loading

jackson-dean commented Sep 24, 2019 •

edited

Loading

jackson-dean commented Sep 24, 2019 •

edited

Loading

jackson-dean commented Nov 20, 2019 •

edited

Loading

jackson-dean commented Nov 20, 2019 •

edited

Loading