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

Fix fetch public/fastboot-fetch.js module definition for Fastboot #167

Merged
merged 2 commits into from
Dec 7, 2018

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Nov 19, 2018

Fix #166, depends on #171

In v6.2.0 fetch/setup was added to public/fastboot-fetch.js so we can setup fetch module with fastboot request info. However this file will be overwritten by any addon's dependency ember-fetch based on last-write-win. To fix this:

  • Rename public/fastboot-fetch.js to public/fetch-fastboot.js to avoid
    lower version ember-fetch overwrite this file.
  • Overrides treeForPublic in index.js to only include public tree if top
    level addon.

For example, top level addon ember-fetch is 6.2.1, transitive dependency is anything before 6.2.0 will result in this dist:

dist
├── assets
│   ├── ...
├── ember-fetch
│   ├── fastboot-fetch.js
│   └── fetch-fastboot.js

6.2.1 as top level addon will manifest.vendorFiles.push('ember-fetch/fetch-fastboot.js'); so the fastboot.js will just be ignored.

This commit also defines fetch module with a setupFastboot method to set host and
protocol for every instance. This could avoid module redefinition.

@xg-wang xg-wang requested review from rwjblue and Turbo87 November 19, 2018 00:48
@Turbo87
Copy link
Member

Turbo87 commented Nov 19, 2018

What if ember-fetch is never the top level addon? What if it is only used as a subdep with several versions?

@xg-wang
Copy link
Member Author

xg-wang commented Nov 19, 2018

@Turbo87 updateFastBootManifest is only called on top level addons. For subdeps only treeForVendor has effect, and we don't calculate cache key for vendor.

So if ember-fetch is only a subdep it only works for browser, and won't have ember-fetch/xx.js in dist/

@xg-wang xg-wang force-pushed the public branch 2 times, most recently from 9c5ab46 to 978f2d8 Compare November 21, 2018 07:47
@Turbo87
Copy link
Member

Turbo87 commented Nov 21, 2018

@xg-wang this PR seems to contain a lot of unrelated changes now. can you make sure to open dedicated PRs for those unrelated changes?

@xg-wang xg-wang force-pushed the public branch 3 times, most recently from 40f4ddb to e970738 Compare December 3, 2018 19:37
@xg-wang
Copy link
Member Author

xg-wang commented Dec 3, 2018

@Turbo87 This PR is now only relying on #171.

@samselikoff
Copy link

#171 is merged, this good to go?

Define fetch module with a setupFastboot method export to set host and
protocol for every instance.

Rename public/fastboot-fetch.js to public/fetch-fastboot.js to avoid
lower version ember-fetch overwrite this file.

Overrides treeForPublic in index.js to only include public asset if top
level addon to avoid any future rename.
@xg-wang xg-wang merged commit 704ec26 into ember-cli:master Dec 7, 2018
@xg-wang xg-wang deleted the public branch December 7, 2018 22:17
@xg-wang xg-wang added the bug label Dec 7, 2018
@runspired
Copy link

We may want to rethink this design a bit now that ember-fetch is included as a dependency by ember-data

@rwjblue
Copy link
Member

rwjblue commented Apr 11, 2019

Agree

@dnalagatla
Copy link
Contributor

+1

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

Successfully merging this pull request may close these issues.

Could not find module fetch/setup imported from prog/instance-initializers/setup-fetch
6 participants