Allow to require module path from whitelisted dependency #200

bobisjan · 2018-10-28T11:48:58Z

Required module path can be one of

fs (built-in Node.js module),
abortcontroller-polyfill (external main module),
abortcontroller-polyfill/dist/cjs-ponyfill (external submodule)
and its scoped @ variants.

FastBoot's build system requires only dependency whitelisting,
thus looking only for the first part of module path.

closes #198

bobisjan · 2018-10-28T11:54:21Z

src/ember-app.js

-          return require(nodeModulesPath);
-        } else {
-          // If it's not on disk, assume it's a built-in node package
-          return require(moduleName);


This branch is not required since resolve.sync will take care of it.

xg-wang · 2018-10-29T01:05:35Z

src/ember-app.js

-          // If it's not on disk, assume it's a built-in node package
-          return require(moduleName);
-        }
+      if (whitelist.indexOf(moduleName) > -1) {


This will be a breaking change, can it be modulePath here?

As far as I can tell, FastBoot supports only dependencies to be whitelisted (eq. abortcontroller-polyfill), not a path to module in dependency (eq. abortcontroller-polyfill/dist/cjs-ponyfill), http://ember-fastboot.com/docs/user-guide#using-whitelisted-node-dependencies.

In ember-fetch, whitelisting of abortcontroller-polyfill/dist/cjs-ponyfill here, caused that the abortcontroller-polyfill dependency was not added (and installed) into build and then requiring module at runtime caused this error ember-cli/ember-fetch#115.

This change allows to specify FastBoot dependency and then require any module from this dependency, so this should not be a breaking change.

That ember-fetch issue is caused by node_modules not inside dist/. If you run the server at the path that has node_modules the module can be installed.

If your change is ~~whitelist.indexOf(moduleName)~~ whitelist.indexOf(modulePath), it will not be a breaking change for any app/addon who's requiring a specific file within a module.

I'm not sure what the official convention for Fastboot is. If users should whitelist a package rather than any subset of its file/module, we can make changes and document, but it's still a breaking change for current whitelisting a module's certain file...

That ember-fetch issue is caused by node_modules not inside dist/. If you run the server at the path that has node_modules the module can be installed.

After ember build and before running server, it's required to cd dist and npm install to have FastBoot whitelisted dependencies available for running, which are part of dist/package.json, eq. like fastboot-s3-downloader does https://github.com/tomdale/fastboot-s3-downloader/blob/master/index.js#L110.

I'm not sure what the official convention for Fastboot is. If users should whitelist a package rather than any subset of its file/module, we can make changes and document, but it's still a breaking change for current whitelisting a module's certain file...

I hope that there is no need to make changes, because it's already said here, that fastbootDependencies contains packages (and Node.js built-ins), based on that I believe that this PR is not introducing any breaking change.

Thanks for the explanation @bobisjan

Yep the dist/node_modules are required, once it's done that issue won't happen.

Definition of "breaking change" is vague here, my argument above was referring to the actual implementation, yours was referring to the documentation. If it is considered wrong for addons/apps to implement based on the current fastboot I can totally accept, just want to point out it will break some addons/apps and they need to fix themselves.

And this PR also need to deal with the scoped package case in this line let moduleName = modulePath.split('/')[0];?

Yep the dist/node_modules are required, once it's done that issue won't happen.

🎉

Definition of "breaking change" is vague here, my argument above was referring to the actual implementation, yours was referring to the documentation. If it is considered wrong for addons/apps to implement based on the current fastboot I can totally accept, just want to point out it will break some addons/apps and they need to fix themselves.

I see, the current implementation (without this PR) based on

https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/lib/broccoli/fastboot-config.js#L86-L111

https://github.com/ember-fastboot/fastboot/blob/master/src/ember-app.js#L117-L136

requires for FastBoot.require('abortcontroller-polyfill/dist/cjs-ponyfill') to work to specify FastBoot dependencies like that

dependecies: { 'abortcontroller-polyfill': '^1.1.9' }, fastbootDependencies: [ 'abortcontroller-polyfill' // (a) 'abortcontroller-polyfill/dist/cjs-ponyfill' // (b) ]

(a) ensures that package itself appears in dist/package.json (1) and will be installed in dist/node_modules (also puts it into whitelist and allows to require it),

(b) puts path into whitelist and allows to require it (2).

With this PR the (b) case will become unnecessary and can be removed, but current behaviour in apps/addons should be preserved.

Looking at https://emberobserver.com/code-search?codeQuery=fastbootDependencies, there are ember-fetch a some firebase related addons, which whitelists without package name.

To be honest I'm curious how these addons can work in FastBoot, because IMO based on our application running on AWS using FastBoot App Server and finding mainmatter/ember-simple-auth#1690, ember-cli/ember-fetch#115, apps/addons with just (b) and without (a) do not work in FastBoot App Server at all today regardless of this PR and need to be fixed anyway.

Maybe users don't use FastBoot App Server and only use ember serve 🤔? If it works (only having (b)) using ember serve then it would break their apps like you mention, will try and report back.

I would like to get feedback from @kratiahuja / @tomdale / @rwjblue / @ember-fastboot on that ☺️.

And this PR also need to deal with the scoped package case in this line let moduleName = modulePath.split('/')[0];?

👍 Good catch, will update, thanks!

stephankaag · 2018-11-19T14:12:23Z

Any progress here? :-) I also ran into this issue recently.

bobisjan · 2018-11-19T16:00:58Z

@stephankaag, I’m going to continue on this during this week.

bobisjan · 2018-11-20T16:23:58Z

@rwjblue, @xg-wang, I've updated PR with scoped packages and deprecation about whitelisting module paths, should be ready for review.

rwjblue · 2018-11-21T15:44:00Z

Looks like one of the other PRs I recently merged introduced conflicts, would you mind rebasing?

kratiahuja · 2018-11-21T19:34:16Z

src/ember-app.js

@@ -116,21 +118,34 @@ class EmberApp {
  buildWhitelistedRequire(whitelist, distPath) {
    whitelist.forEach(function(whitelistedModule) {
      debug("module whitelisted; module=%s", whitelistedModule);
+
+      // Remove after 2.0.0


Curious to understand, why are we removing this after 2.0? This may be a backward incompatible change for apps that are relying on this.

This is intended only for fastboot 1.x, with 2.0 all apps/addons should be using only package names in the whitelist, so it should be unnecessary here.

Do you think that schema version check should be used instead?

kratiahuja · 2018-11-21T19:35:23Z

src/ember-app.js

+      let packageName = getPackageName(whitelistedModule);
+
+      if (packageName !== whitelistedModule) {
+        console.warn("[DEPRECATION] Whitelisting module path '" + whitelistedModule + "' will not be supported after fastboot#2.0.0 and it should be removed from the whitelist.");


I think in fastboot we have version maintainence between ember-cli-fastboot and fastboot. You can see here: https://github.com/ember-fastboot/fastboot/blob/master/src/fastboot-schema-versions.js. I think we should use that versus version of fastboot. This can be confusing as ember-cli-fastboot is already on 2.0.0.

Thanks 👍, will update accordingly.

Required module path can be one of - `fs` (built-in Node.js module), - `abortcontroller-polyfill` (external main module), - `abortcontroller-polyfill/dist/cjs-ponyfill` (external submodule). FastBoot's build system requires only dependency whitelisting, thus looking only for the first part of module path.

bobisjan · 2018-11-22T11:37:44Z

@kratiahuja, I've updated PR with new schema version and created follow up for ember-cli-fastboot ember-fastboot/ember-cli-fastboot#648.

Also added a test describing expected behaviour - whitelisting foo package and requiring both FastBoot.require('foo') and FastBoot.require('foo/bar') works (without adding foo/bar into fastbootDependencies).

kratiahuja · 2018-11-29T21:48:16Z

Hi @bobisjan , I am very sorry I somehow missed reviewing the changes here. This looks good to me. I'll merge this. Please let me know when you want me to do a release.

rwjblue · 2018-11-30T10:23:32Z

@kratiahuja - AFAICT this is not a breaking change (please double check my thinking), we should probably just release right away...

kratiahuja · 2018-12-14T19:47:30Z

@rwjblue Yup this doesn't look like a breaking change any longer to me. I'll release it today.

html-oriented manifest format ----------------------------- In the new schema format, which is defined in ember-fastboot/fastboot@3fd5bc9 the manifest is written into HTML and later extracted by fastboot on server side instead of previously reading from dist/package.json Note: The new schema in fastboot does not handle fastboot config https://github.com/ember-fastboot/ember-cli-fastboot/tree/e4d0b7c7bcdf82def0dc8726835b49d707673f41#providing-additional-config This commit changes to read Fastboot.config from dist/package.json instead of ignoring it Allow to require module path from whitelisted dependency ------------------------------------------------------- Incrementing schema to 5 also included the changes in schema 4 strictWhitelist See ember-fastboot/fastboot#200

html-oriented manifest format ----------------------------- In the new schema format, which is defined in ember-fastboot/fastboot@3fd5bc9 the manifest is written into HTML and later extracted by fastboot on server side instead of previously reading from dist/package.json Note: The new schema in fastboot does not handle fastboot config https://github.com/ember-fastboot/ember-cli-fastboot/tree/e4d0b7c7bcdf82def0dc8726835b49d707673f41#providing-additional-config This commit changes to read Fastboot.config from dist/package.json instead of ignoring it Allow to require module path from whitelisted dependency ------------------------------------------------------- Incrementing schema to 5 also included the changes in schema 4 strictWhitelist See ember-fastboot/fastboot#200 Revert back to put config in dist/package.json add data-fastboot-ignore to unexpected files properly ignore files that should not execute in fastboot

html-oriented manifest format ----------------------------- In the new schema format, which is defined in ember-fastboot/fastboot@3fd5bc9 the manifest is written into HTML and later extracted by fastboot on server side instead of previously reading from dist/package.json Note: The new schema in fastboot does not handle fastboot config https://github.com/ember-fastboot/ember-cli-fastboot/tree/e4d0b7c7bcdf82def0dc8726835b49d707673f41#providing-additional-config The support is added in the #854. Allow to require module path from whitelisted dependency ------------------------------------------------------- Incrementing schema to 5 also included the changes in schema 4 strictWhitelist See ember-fastboot/fastboot#200 Revert back to put config in dist/package.json add data-fastboot-ignore to unexpected files properly ignore files that should not execute in fastboot

bobisjan commented Oct 28, 2018

View reviewed changes

bobisjan force-pushed the require-module-path branch from 2518995 to a8bea25 Compare October 28, 2018 11:56

bobisjan mentioned this pull request Oct 28, 2018

Use package name of AbortController polyfill in whitelisted dependencies ember-cli/ember-fetch#137

Merged

xg-wang reviewed Oct 29, 2018

View reviewed changes

bobisjan force-pushed the require-module-path branch 5 times, most recently from 1aea993 to fd642f3 Compare November 20, 2018 16:13

rwjblue requested a review from kratiahuja November 21, 2018 15:44

bobisjan force-pushed the require-module-path branch from fd642f3 to 41b058b Compare November 21, 2018 15:50

kratiahuja requested changes Nov 21, 2018

View reviewed changes

bobisjan force-pushed the require-module-path branch 2 times, most recently from 43c2444 to 6fcc407 Compare November 22, 2018 10:17

bobisjan force-pushed the require-module-path branch from 6fcc407 to 708e8da Compare November 22, 2018 10:20

bobisjan mentioned this pull request Nov 22, 2018

Update schema version to support strict whitelist ember-fastboot/ember-cli-fastboot#648

Open

kratiahuja approved these changes Nov 29, 2018

View reviewed changes

kratiahuja merged commit 4516747 into ember-fastboot:master Nov 29, 2018

xg-wang mentioned this pull request Nov 30, 2018

Setup addon tests ember-cli/ember-fetch#171

Merged

rwjblue added the bug label Dec 10, 2018

xg-wang mentioned this pull request Feb 2, 2019

Error: Cannot find module 'abortcontroller-polyfill/dist/cjs-ponyfill' ember-cli/ember-fetch#184

Closed

bobisjan deleted the require-module-path branch February 16, 2019 20:20

xg-wang mentioned this pull request Apr 24, 2019

Issue with fastboot after upgrading to ember 3.9 ember-fastboot/ember-cli-fastboot#686

Closed

xg-wang mentioned this pull request May 13, 2019

FastBoot.require is broken when running locally #221

Closed

xg-wang mentioned this pull request Jul 22, 2019

ember-cli-fastboot with ember-fetch broken ember-fastboot/ember-cli-fastboot#711

Closed

xg-wang mentioned this pull request Apr 7, 2021

Increment latest schema version to 5 ember-fastboot/ember-cli-fastboot#822

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to require module path from whitelisted dependency #200

Allow to require module path from whitelisted dependency #200

bobisjan commented Oct 28, 2018 •

edited

Loading

bobisjan Oct 28, 2018

xg-wang Oct 29, 2018

bobisjan Oct 29, 2018

xg-wang Oct 29, 2018

bobisjan Oct 29, 2018

xg-wang Nov 1, 2018

bobisjan Nov 1, 2018

stephankaag commented Nov 19, 2018

bobisjan commented Nov 19, 2018

bobisjan commented Nov 20, 2018

rwjblue commented Nov 21, 2018

kratiahuja Nov 21, 2018

bobisjan Nov 22, 2018

kratiahuja Nov 21, 2018

bobisjan Nov 22, 2018

bobisjan commented Nov 22, 2018

kratiahuja commented Nov 29, 2018

rwjblue commented Nov 30, 2018

kratiahuja commented Dec 14, 2018

Allow to require module path from whitelisted dependency #200

Allow to require module path from whitelisted dependency #200

Conversation

bobisjan commented Oct 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephankaag commented Nov 19, 2018

bobisjan commented Nov 19, 2018

bobisjan commented Nov 20, 2018

rwjblue commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobisjan commented Nov 22, 2018

kratiahuja commented Nov 29, 2018

rwjblue commented Nov 30, 2018

kratiahuja commented Dec 14, 2018

bobisjan commented Oct 28, 2018 •

edited

Loading