search

embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
339 stars 137 forks source link

Embroider build replaces Fastboot generated package.json #160

Closed dnalagatla closed 4 years ago

dnalagatla commented 5 years ago

My PR - https://github.com/ember-fastboot/ember-cli-fastboot/pull/690 moved the implementation form the postProcessTree to treeForPublic hook. But after doing that, I noticed embroider build doesn't add the package.json generated by ember-cli-fastboot addon. Instead, it keeps the package.json created by enbroider build in the dist folder. I am thinking some diff code is assuming the package.json generated as part of Fastboot tree is similar to package.json generated by embroider build and keeps embroider build related package.json

To Repro the issue

  1. clone - https://github.com/dnalagatla/my-embroider-test.git
  2. run yarn install
  3. ember s
  4. launch http://localhost:4200

page will show following error:

Error: /var/folders/tv/fgbbtzvd1f1dbv45y7r8l09m000ty_/T/broccoli-82821i0tVejhn4W1b/out-176-packager_runner_embroider_webpack/package.json was malformed or did not contain a manifest. Ensure that you have a compatible version of ember-cli-fastboot.
    at EmberApp.readPackageJSON (/Users/dnalagat/DEVELOPMENT/ember-cli-fastboot/node_modules/fastboot/src/ember-app.js:362:13)
    at new EmberApp (/Users/dnalagat/DEVELOPMENT/ember-cli-fastboot/node_modules/fastboot/src/ember-app.js:34:23)
    at FastBoot._buildEmberApp (/Users/dnalagat/DEVELOPMENT/ember-cli-fastboot/node_modules/fastboot/src/index.js:114:17)
    at new FastBoot (/Users/dnalagat/DEVELOPMENT/ember-cli-fastboot/node_modules/fastboot/src/index.js:52:10)
    at app.use (/Users/dnalagat/DEVELOPMENT/ember-cli-fastboot/index.js:318:29)
    at Layer.handle [as handle_request] (/Users/dnalagat/DEVELOPMENT/test-pemberly-bpr/test-ember/my-embroider-test/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/dnalagat/DEVELOPMENT/test-pemberly-bpr/test-ember/my-embroider-test/node_modules/express/lib/router/index.js:317:13)
    at /Users/dnalagat/DEVELOPMENT/test-pemberly-bpr/test-ember/my-embroider-test/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/Users/dnalagat/DEVELOPMENT/test-pemberly-bpr/test-ember/my-embroider-test/node_modules/express/lib/router/index.js:335:12)
    at next (/Users/dnalagat/DEVELOPMENT/test-pemberly-bpr/test-ember/my-embroider-test/node_modules/express/lib/router/index.js:275:10)

This error is thrown because manifest property is missing in generated package.json.

I am thinking the fastboot generation of package.json should move to postBuild hook instead of using treeForPublic. what do you guys think @ef4 @stefanpenner @rwjblue

Related to issue - https://github.com/embroider-build/embroider/issues/112 I missed this error during testing of my PR as the git repo mentioned in the above issue didn't include fastboot and I tested assuming ember-cli-fastboot was included as dependency. :(

ef4 commented 5 years ago

Moving to postBuild won't make compatibility with current embroider any easier. AFAIK we don't even run that hook.

I think we should deal with this in embroider. Probably by detecting the package.json in the public tree and merging it into our own.

dnalagatla commented 5 years ago

Hi @ef4, @rwjblue and myself investigated where embroider build is overwriting package.json generated by ember-cli-fastboot. I created this PR to merge package.json properties into package.json created by embroider.

dnalagatla commented 5 years ago

After adding the above fix in the test app I noticed following issues with Embroider+Fastboot build:

Investigating ^ issues. Will update here. cc: @stefanpenner @rwjblue @ef4

ef4 commented 5 years ago

This was closed by #178.

ef4 commented 5 years ago

Reopened because the fix was reverted.

jelhan commented 4 years ago

Should be fixed by #383 if I didn't missed something.

simonihmig commented 4 years ago

Although the PR mentioned above seems to address the issue, I am still running into it, with the latest embroider release (0.27.0).

You can see that quite well when building the fastboot-app test-package in this repo: dist/package.json contains none of the FastBoot manifest stuff that you see in a classic build!

In an embroider build (ember build):

package.json ``` { "name": "fastboot-app", "version": "0.0.0", "private": true, "description": "Small description for fastboot-app goes here", "repository": "", "license": "MIT", "author": "", "directories": { "doc": "doc", "test": "tests" }, "scripts": { "build": "ember build --environment=production", "lint": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*", "lint:hbs": "ember-template-lint .", "lint:js": "eslint .", "start": "ember serve", "test": "npm-run-all lint:* test:*", "test:ember": "ember test --test-port=0", "test:ember-production": "ember test --test-port=0 --environment=production", "test:fastboot": "qunit fastboot-tests", "test:fastboot-production": "FASTBOOT_APP_PROD=true qunit fastboot-tests" }, "devDependencies": { "@ember/optional-features": "^1.3.0", "@embroider/compat": "0.27.0", "@embroider/core": "0.27.0", "@embroider/sample-lib": "0.0.0", "@embroider/webpack": "0.27.0", "@glimmer/component": "^1.0.0", "@glimmer/tracking": "^1.0.0", "babel-eslint": "^10.1.0", "broccoli-asset-rev": "^3.0.0", "ember-auto-import": "^1.5.3", "ember-cli": "~3.18.0", "ember-cli-app-version": "^3.2.0", "ember-cli-babel": "^7.20.5", "ember-cli-dependency-checker": "^3.2.0", "ember-cli-fastboot": "^2.2.2", "ember-cli-fastboot-testing": "^0.4.0", "ember-cli-htmlbars": "^4.3.1", "ember-cli-inject-live-reload": "^2.0.2", "ember-cli-sri": "^2.1.1", "ember-cli-uglify": "^3.0.0", "ember-export-application-global": "^2.0.1", "ember-fetch": "^8.0.1", "ember-load-initializers": "^2.1.1", "ember-maybe-import-regenerator": "^0.1.6", "ember-qunit": "^4.6.0", "ember-resolver": "^8.0.0", "ember-source": "~3.18.0", "ember-template-lint": "^2.6.0", "ember-welcome-page": "^4.0.0", "eslint": "^6.8.0", "eslint-plugin-ember": "^8.4.0", "eslint-plugin-node": "^9.0.0", "fastboot": "^3.1.0", "fastboot-addon": "0.0.0", "jsdom": "^16.2.2", "loader.js": "^4.7.0", "npm-run-all": "^4.1.5", "qunit": "^2.10.0", "qunit-dom": "^1.2.0" }, "engines": { "node": "10.* || >= 12" }, "ember": { "edition": "octane" }, "volta": { "extends": "../../package.json" }, "keywords": [ "ember-addon" ], "ember-addon": { "type": "app", "version": 2, "assets": [ "/assets/fastboot-app.css", "ember-cli-fastboot/app-factory.js", "ember-fetch/fetch-fastboot.js", "ember-welcome-page/images/construction.png", "fastboot-addon/sample.js", "assets/embroider_macros_fastboot_init.js", "robots.txt", "testem.js", "index.html", "tests/index.html", "package.json", "assets/vendor.map", "assets/vendor.css.map", "assets/test-support.map", "assets/test-support.css.map" ], "template-compiler": { "filename": "_template_compiler_.js", "isParallelSafe": true }, "babel": { "filename": "_babel_config_.js", "isParallelSafe": true, "majorVersion": 7, "fileFilter": "_babel_filter_.js" }, "resolvable-extensions": [ ".wasm", ".mjs", ".js", ".json", ".hbs" ], "root-url": "/", "auto-upgraded": true }, "dependencies": { "abortcontroller-polyfill": "^1.4.0", "node-fetch": "^2.6.0" }, "fastboot": { "schemaVersion": 5, "htmlEntrypoint": "index.html", "moduleWhitelist": [ "node-fetch", "abortcontroller-polyfill", "abortcontroller-polyfill/dist/cjs-ponyfill" ] } } ```

In a classic build (CLASSIC=true ember build):

package.json ``` { "dependencies": { "abortcontroller-polyfill": "^1.4.0", "node-fetch": "^2.6.0" }, "fastboot": { "appName": "fastboot-app", "config": { "fastboot-app": { "APP": { "autoboot": false, "name": "fastboot-app", "version": "0.0.0+296dd523" }, "EmberENV": { "EXTEND_PROTOTYPES": { "Date": false }, "FEATURES": {}, "_APPLICATION_TEMPLATE_WRAPPER": false, "_DEFAULT_ASYNC_OBSERVERS": true, "_JQUERY_INTEGRATION": false, "_TEMPLATE_ONLY_GLIMMER_COMPONENTS": true }, "environment": "development", "exportApplicationGlobal": true, "fastboot": { "hostWhitelist": [ "/localhost:\\d+$/" ] }, "locationType": "auto", "modulePrefix": "fastboot-app", "rootURL": "/" } }, "hostWhitelist": [ "/localhost:\\d+$/" ], "manifest": { "appFiles": [ "assets/fastboot-app.js", "assets/fastboot-app-fastboot.js" ], "htmlFile": "index.html", "vendorFiles": [ "assets/vendor.js", "ember-fetch/fetch-fastboot.js", "fastboot-addon/sample.js" ] }, "moduleWhitelist": [ "node-fetch", "abortcontroller-polyfill", "abortcontroller-polyfill/dist/cjs-ponyfill" ], "schemaVersion": 3 } } ```

The error that I see is not the one mentioned in the original comment, but rather You must provide a hostWhitelist to retrieve the host when trying to read request.host. But this is also caused by the package.json being not what Fastboot expects, specifically the missing hostWhitelist field.

Will provide a PR with a failing test...

simonihmig commented 4 years ago

Here is a failing test: https://github.com/embroider-build/embroider/pull/574

ef4 commented 4 years ago

I don't think your problem is related to this issue. In your package.json, you can see that we did emit fastboot info, it's just that it's using schemaVersion: 5 which is much smaller (because it relies on the contents of index.html instead of duplicating that info inside package.json).

simonihmig commented 4 years ago

You are right, I didn't pay enough attention to the FastBoot manifest data it seems. It was just obvious that it was totally different, so I wrongly assumed Embroider had omitted something. Until I realized that Embroider has some custom adapter for ember-cli-fastboot, which seems to explain the vast difference between builds.

I think I have found a fix, will update my PR, then let's move the discussion over there...

So, as my issue is indeed not really related to this issue, which seems to have been fixed by now, I think we can close this...