feat(error message): Especifing error message when cross-boundary

[Grubba27]

Relates to this #11378

TODO / For discussion:

• Implementation
• Adding tests

[StorytellerCZ]

As for tests I recall that there are tests that test that Meteor creates a project properly and so on, so I would look to those for inspiration on how to test this.

[zodern]

The file packages/modules-runtime-hot/installer.js is a modified version of https://github.com/benjamn/install that is only used on the client when HMR is enabled.

When install is unable to find a module, it calls a fallback function. Meteor defines these in the modules-runtime package, and this is where we can put any Meteor-specific errors. For example, this is where it creates a better error message if you tried to import a meteor package that isn't available. This will also allow having it work differently on each arch (for example, on the server it should use the original error if you try to import a non-existent file in a server folder).

To avoid confusing developers, I think it should be more precise in when it uses this error instead of the default Cannot find module error. Calling require for these paths should use the default error:

• /imports/client/non-existent-file.js on the client
• /server/non-existent-file.js on the server

Valid imports on any arch:

• meteor/client
• meteor/server
• lodash/client/index.js
• lodash/server/index.js
• /imports/client.js

There are some situations where it isn't clear what the developer intended.
For example, importing./client on the server. This could be intended as ./client.js, which is valid, or ./client/index.js, which is not supported. Have you considered which error is better to show in this situation?

[zodern]

For tests, you could add them to the modules self tests: https://github.com/meteor/meteor/blob/devel/tools/tests/modules.js

Or add them to the modules-runtime tests: https://github.com/meteor/meteor/blob/devel/packages/modules-runtime/modules-runtime-tests.js.

The modules-runtime would be easier. You can call meteorInstall to get the require function, and then test passing different paths to require to check what error it throws.

[Grubba27]

Hey @zodern, I've done some upgrades based on your comments, but I got stuck in how to make modules work inside modules-runtime. I wanted to make these newer implementations using modern javascript or even typescript. I've tried using module.exports and require, but it also did not work have you ever faced something like this? Bellow, there's the error message.

For example, importing./client on the server. This could be intended as ./client.js, which is valid, or ./client/index.js, which is not supported. Have you considered which error is better to show in this situation?

• No I have not. Do you have any suggestions? I want to make small steps into this more developer-friendly Meteor. I guess the simpler, the better, would stay with the default error for now but, given time, make the errors more accurate. What do you think?

[zodern]

Unfortunately, modules-runtime can not use modules since it is one of the packages that enables other packages to use modules, and it can not use newer js syntax (at least in any files used in the legacy client) since it must load before the ecmascript and typescript packages.

No I have not. Do you have any suggestions? I want to make small steps into this more developer-friendly Meteor. I guess the simpler, the better, would stay with the default error for now but, given time, make the errors more accurate. What do you think?

I think using the default message in this case sounds good.

[Grubba27]

Unfortunately, modules-runtime can not use modules since it is one of the packages that enables other packages to use modules, and it can not use newer js syntax (at least in any files used in the legacy client) since it must load before the ecmascript and typescript packages.

• So using standard require and module.exports should work? I should focus on using features before es6 right ?

[zodern]

So using standard require and module.exports should work? I should focus on using features before es6 right ?

require and module.exports are features of modules, so they will not work. The packages that are unable to use modules instead use globals; when Meteor builds the package, it turns them into pseudo globals so they are only available within the package.

Yes, all es5 syntax should be fine.

[Grubba27]

ci is green now :)

[henriquealbert]

LGTM 🚀

[Grubba27]

Could not figure out why tests on the server fall on standard error while ones for clients fall on the new ones
any ideas @zodern ?

[Grubba27]

Made the tests work properly. On the client, it throws the correct error messages that were the focus of this PR would you like to add something @zodern?

[zodern]

I think there are 3 things left from what we had previously discussed:

• On the server it should only check for the client folder, and on the client it should only check for the server folder. Currently it does the same checks on both the client and server. The client test modules.throwServerError shows this - currently it throws the cross-boundary import error, but it should instead throw the Cannot find module error.
• We had decided in cases where importing things like /imports/client where it isn't clear if it is supposed to be a file or a folder to throw the Cannot find module error. For this, you can simply have it always ignore the last part of the path when checking if the path includes a folder.
• It shouldn't throw a cross-boundary import error if the path includes node_modules or doesn't start with a . or / since the restrictions don't affect npm packages

[Grubba27]

Now I think I got everything. the only thing that I'm thoughtful is the order of the validations:

if(!(id.startsWith('.') || id.startsWith('/'))) {
    throw err;
  }

  if (id.endsWith('client') || id.endsWith('server')) {
    // We don't know for sure what client wants to do so throw standard error
    throw err;
  }

  if (imports(id).from('node_modules')) {
    // Problem with node modules
    throw err;
  }

there I'm not sure if there was supposed to be an order (left this way because I think is more concise)

[zodern]

I think it is almost ready. The order doesn't really matter for those checks.

Checking if the path ends with client or server correctly handles paths like ./client, but will cause the wrong error to be thrown for paths like /server/graphql/client where we can tell from other parts of the path that it is a cross boundary import. A more accurate way to handle this would be to have imports.from ignore the last part of the path. For example:

let parts = id.split('/');
// Ignore last part since there is no way of knowing if it is a folder or file
parts.pop();

[Grubba27]

I've done this adjustment! and added some tests for the case that I've mentioned