Implement imports

[izaera]

Now that we have explicits exports (#516) we need to be able to import packages from a different OSGI bundle.

The "problem" is that there's something incorrect with the imports in bundler 2.x that we can fix for bundler 3.x but at the cost of introducing a big interoperability burden that will be with us until everybody migrates everything to bundler 3.x.

Let's see the problem. Imagine you create the react-portlet and react-provider projects (both at version 1.0.0) where the second exports the React framework (version 16.12.0) so that the first one imports it.

This would lead to the following module defined in the AMD loader:

react-provider$react@16.12.0/index

Which would be imported by react-portlet by means of the .npmbundlerrc.

This looks good, but pay attention to the name of the module: it contains React's version (16.12.0) but not react-provider's version (1.0.0). Thus, if we deploy another version of react-provider (which is legal in OSGi) there will be a collision if it also contains react@16.12.0.

We have never seen any problem related to this because the scenario is quite strange and even if it happened, the loader would choose one of the two react@16.12.0 copies and, if both are the same (which is expected), nothing would happen.

However it is tempting to fix it for bundler 3.x, not only to avoid the error, but to simplify how the project is configured and built a lot (see next comment).

[izaera]

In bundler 2, for imports to happen configuration is quite complex. In particular, you need to specify react's version two times in react-portlet: one in package.json and another in .npmbundlerrc, like this:

package.json

{
  "dependencies": {
    "react": "^16.0.0"
  }
}

.npmbundlerrc

{
  "config": {
    "imports": {
      "react-provider": {
        "react": "^16.0.0"
      }
    }
  }
}

Note that in this case we use the same semver expression in both files, but it could be different because the first one is just used by the project during build time and the second one during runtime. You can even remove the dependency from the package.json file and nothing will fail during runtime!

This leads to several strategies to handle project and imports versions which adds complexity and separates from the standard expected frontend development practices (though it is obvious that we will need some deviation as the imports are not a standard feature).

In an ideal world we would like to specify everything in package.json and simply tell the bundler where to get dependencies from during runtime. And then leave everything else to the bundler/platform.

We'll see how this can be achieved in bundler 3.x in the next comment.

[izaera]

Imagine if we could be able to configure imports like this:

package.json

{
  "dependencies": {
    "react": "^16.0.0",
    "react-provider": "1.0.0"
  }
}

.npmbundlerrc

{
  "imports": {
    "react": "react-provider"
  }
}

Wouldn't it be much simpler?

Doing it this way we would be able to enforce version constraints during build time because we would be able to compare react-provider's React version with the one in react-portlet and see if they are compatible, failing the build otherwise.

The only limitation of this model is that react-provider must exist as a npm package. In the times of bundler 2 we weren't able to satisfy that because the Portal OSGi modules were not true npm packages, but now that we use yarn for the build this limitation doesn't hold any more so I guess we could go this way... :thinking:

Also, if we iterate a bit over this idea, we can get rid of the dependency with react in react-portlet given that react-provider already provides it transitively. In that case we wouldn't even need to check anything, because the project would be built with the same version it is run.

[izaera]

So, if we decide to go that way, we would configure something like:

package.json

{
  "dependencies": {
    "react-provider": "^1.0.0"
  }
}

.npmbundlerrc

{
  "imports": {
    "react": "react-provider"
  }
}

But note how, in this case react-provider has a ^ like semver constraint which is the usual way to go when we want a project to be automatically updated. But, because this semver constraint is resolved during runtime, it may lead to an update of react version during runtime when the project has been built with a previous one (think what would happen if the project was built with react-provider@1.0.0 pulling react@16.10.0 but then, in the portal, someone deploys react-provider@1.1.0 which pulls react@16.12.0).

Do we really want that? :thinking: I would say we can go that way because if the user doesn't want that to happen he can always use 1.0.0 instead of ^1.0.0, but I admit deciding one thing or the other may lead to a less error prone platform.

[izaera]

Just to be exhaustive, the way this would be implemented would be, instead of react-provider exporting a module named react-provider$react@16.12.0/index it would simply export something like react-provider@1.0.0/$package-export$/react.

With this configuration, nobody needs to specify react-provider's react version any more because it is unambiguous: knowing the version of react-provider, the version of react can be derived without problem.

And because the current platform already resolves semver constraints, if react-portlet specifies react-provider@^1.0.0 it would resolve to react-provider@1.1.0, react-provider@1.2.0 and so on automatically during runtime.

Also, the setup would be much more simple because all deployed bundles would look like projects without any third party dependency. This is because react-provider@1.0.0/$package-export$/react looks like an internal module of react-provider to the outside world, not as a third party dependency like it did in bundler 2 architecture. This means that if everything was expressed like that (bundler 3.x model), we could get rid of a lot of code in the portal too.

The only downside is that if we wanted to interoperate bundler 2 and bundler 3 OSGi bundles we would need to specify if what we are importing has bundler 2 like exports or bundler 3 which looks quite ugly :disappointed:.

The same would happen with exports, though it will be less problematic because we can just export things both bundler 2 and 3 way on user's demand.

[wincent]

Some random thoughts on this.

The scenario you're trying to address here is so vanishingly unlikely (deploying two different versions of an OSGi module containing the same version of a dependency), and the likely consequences so benign, that we should make sure that our response to it is proportional. In other words, I think your arguments in favor of making things "simpler" are more persuasive than the one about handling the edge case.

Also, if we iterate a bit over this idea, we can get rid of the dependency with react in react-portlet given that react-provider already provides it transitively.

I am not a fan of making transitive dependencies hidden like this. Explicit is good. As a rule of thumb, I believe that if you have an import 'thing' in your source code, then there should be a thing in your package.json saying what version you expect. Now, if you want that version to be super loose because it will make your life easier that's fine by me, because then you're declaring "I need react, and I am happy to go with the version supplied by react-provider":

package.json

{
    "dependencies": {
        "react": "*",
        "react-provider": "^1.0.0"
    }
}

The other idea we talked about elsewhere was that these imports-based dependencies are technically peerDependencies and should be declared as such:

package.json

{
    "dependencies": {
        "react-provider": "^1.0.0"
    },
    "peerDependencies": {
        "react": "*"
    }
}

Doesn't exactly meet your goal of "simple" but it is very precise.

In any case, no matter what we do, I like the idea of simplifying what's in the .npmbundlerrrc.js:

.npmbundlerrc.js

module.exports = {
    imports: {
        react: 'react-provider',
    }
};

And if, for some reason, you wanted to select from multiple possible versions of the provider:

module.exports = {
    imports: {
        react: ['react-provider', '^3.0.0'],
    }
};

or some similar format: an object, a string ('react-provider@^3.0.0') etc.

I haven't thought as much as you about this, but I also wonder if we could streamline what seems to be the most common case:

I want OSGi modules A, B, C to depend on common code in shared module D rather than duplicating it.

Rather than catering to the more complex case which I am not even sure is one that people run into in the real world:

I have different versions of a package in modules A, B (etc), and from modules X, Y, Z I want to be able to grab a specific version from one of A, B, C...

If what you mostly care about is the first case, then there could be something in the metadata of react-provider that declares that it is a "provider" module, and that would mean that any project that had react-provider in a dependencies could be assumed to want to import the provider's exports rather than having their own. That would be "zero-config" in a sense (other than adding react-provider to your package.json), and you'd only have to provide settings in an .npmbundlerrc.js if you wanted to step out of that common use case.

[izaera]

I like the idea of putting something in react-provider so that react-portlet knows it exports react. Then, the bundler, by default, would import react from react-provider unless the user configures something in .npmbundlerrc to opt-out.

In cases where react-provider is not available through the npm registry, the developers can resort to using yarn or lerna (even npm link :scream:) to be able to make react-portlet depend on react-provider.

[izaera]

Regarding how to express the dependency of react-portlet with react we can leave it up to the user, as it doesn't matter (for the bundler) if you omit it, add it as a peerDependency or semver contrain it to "*" in dependencies.

There's one corner case, however, that we should probably check (while bundling). It is possible that the user may configure something like:

package.json

{
  "dependencies": {
    "react": "16.0.0",
    "react-provider": "1.0.0"
  }
}

In this case, npm would pull two react copies: 16.12.0 and 16.0.0, putting the first one in the node_modules subfolder or react-provider and 16.0.0 in the project's node_modules.

In the case of bundling that would equate to not importing react from react-provider but bundling react 16.0.0 locally into the project and using it (instead of importing).

However, we should probably warn the user because it smells a lot like an error, not something really wanted. In addition we should add some way to tell the bundler that we really want it that way in case the user really configured that on purpose (probably the imports opt-out explained in the previous comment would be enough).

[izaera]

It looks like we have the solution to the imports/exports mechanism during build time.

Still, we need to decide how modules will be exported during runtime, i.e., like in bundler 2 (react-provider$react@16.12.0/index) or the new way (react-provider@1.0.0/$package-export$/react).

We said that conflicts because of the react-provider namespace not containing the version looks very improbable and even if they happen nothing would break. The situation in bundler 3, however, is a bit different because not everything is exported, just what the user configures, and everything else is bundled with webpack in an opaque .js file.

Nevertheless, if someone exports the same entry points in two different versions of the same provider, it should work equally well because nothing would be missing (as each entry point has everything it needs inside its webpack bundle).

There could be cases where the internal structure of the webpack bundling would be different but it would still work and the only downside would be that debugging would be a bit tricky. I don't see this as a big problem, but it still poses the risk that someone has a setup where they want to deploy two versions of the same provider and insist in being able to decide which one to import.

Still, I don't see why someone would want to deploy two versions of the same provider and, if someone does, we may forbid it in the portal and say it is a limitation.

So, if we agree on this, we can use bundler 2 structure which has the big benefit of being backward compatible (even though it's a bit more complex).

[izaera]

One more thing we should think about is how to export modules in packages with multiple entry points (like lodash, which has one different module per API function).

I would say we can configure things like lodash/* in the exports section of the .npmbundlerrc file but I'm still worried about the performance because that would create one webpack bundle per lodash module plus its entry point for the AMD loader which looks like it's a much more inefficient setup than the one in bundler 2. However, it may not be the case, and maybe webpack is smart enough to make it faster than bundler 2 even in this case. If not, we would need to think some way to switch between the two type of builds.

[wincent]

However, we should probably warn the user because it smells a lot like an error, not something really wanted. In addition we should add some way to tell the bundler that we really want it that way in case the user really configured that on purpose (probably the imports opt-out explained in the previous comment would be enough).

One thing to bear in mind is that this sort of thing (matching version numbers) is already enforced in liferay-portal by SF, I think in JSONPackageJSONDependencyVersionCheck.

So, if we agree on this, we can use bundler 2 structure which has the big benefit of being backward compatible (even though it's a bit more complex).

Sounds reasonable to me. One thing though:

We said that conflicts because of the react-provider namespace not containing the version looks very improbable and even if they happen nothing would break.

I don't think that's strictly true — it's more accurate to say that "things are unlikely to break". It is certainly possible that the NPM module wonderful-package, does something that is supposed to be globally unique (like setting up a singleton registry inside its module closure), and when you have two supposedly identical copies of wonderful-package loaded somewhere, your registry that thinks it is a singleton and can safely do globally visible things like attaching stuff to window or document.body etc is actually unsafe because its assumption about being a singleton is invalid.

I would say we can configure things like lodash/* in the exports section of the .npmbundlerrc file but I'm still worried about the performance

I say try it and see what happens. 😂

[izaera]

One thing to bear in mind is that this sort of thing (matching version numbers) is already enforced in liferay-portal by SF, I think in JSONPackageJSONDependencyVersionCheck.

That's much better (for us). But I was thinking in people outside.

[wincent]

Yeah, I figured that (which is why I mentioned liferay-portal). Just pointing out that this might be reinventing a wheel.

[jbalsas]

I like the idea of putting something in react-provider so that react-portlet knows it exports react. Then, the bundler, by default, would import react from react-provider unless the user configures something in .npmbundlerrc to opt-out.

Don't we have this already inside imports in the config file?

In cases where react-provider is not available through the npm registry, the developers can resort to using yarn or lerna (even npm link 😱) to be able to make react-portlet depend on react-provider.

This is okay for us, but it would be a big problem for external developers. Right now, they don't really need real npm packages, but now they will... which means we'll need to either publish them or think of an alternative...

One more thing we should think about is how to export modules in packages with multiple entry points (like lodash, which has one different module per API function).

This might also fall under the "simplification clause". We might not need to try to cover this right now when in reality the amount of exported entries we need might be limited and we're not really doing this for lodash anyways.

I would prefer to check some of our exported libs (React, Recharts, react-dnd, Angular...) and see if those are good examples of this problem.

[jbalsas]

Left this for the end:

So, if we agree on this, we can use bundler 2 structure which has the big benefit of being backward compatible (even though it's a bit more complex).

At this point... I think we would benefit more from something backwards compatible than from something that's strictly correct. Specially if that means we can use the same toolchain across different versions of DXP

[izaera]

I like the idea of putting something in react-provider so that react-portlet knows it exports react. Then, the bundler, by default, would import react from react-provider unless the user configures something in .npmbundlerrc to opt-out.

Don't we have this already inside imports in the config file?

Yes, it is in .npmbundlerrc, but because it has now changed to liferay-npm-bundler.config.js and it's executable, we need some place to put it in a declarative way (maybe in a liferay/exports section in package.json or something like that).

Otherwise, it would be impossible to read it from outer projects because you may need the whole source code to be able to execute liferay-npm-bundler.config.js which is something that will not be available in outer projects consuming the provider.

In cases where react-provider is not available through the npm registry, the developers can resort to using yarn or lerna (even npm link 😱) to be able to make react-portlet depend on react-provider.

This is okay for us, but it would be a big problem for external developers. Right now, they don't really need real npm packages, but now they will... which means we'll need to either publish them or think of an alternative...

The alternatives are lerna, yarn, npm link, or a local registry (like, for example, Verdaccio). I agree that it may turn it more complex for some people with very specific setups, but the rest of the world it should simplify things a lot.

What I expect (and that's what we've seen until now in customer projects) is that providers and consumers are inside the same repo or next to each other. In fact, if someone has a setup where the providers are done by some other people than the consumers, they should consider installing a local npm registry, as that means that they are inside a project where responsibilities spread across different distributed teams and they need some way to integrate all together.

However, this is my assumption. I may be wrong because we don't see every project out there.

One more thing we should think about is how to export modules in packages with multiple entry points (like lodash, which has one different module per API function).

This might also fall under the "simplification clause". We might not need to try to cover this right now when in reality the amount of exported entries we need might be limited and we're not really doing this for lodash anyways.

Yes. We will not care about this in this phase of the project, but we should bear it in mind because if we take decisions that prevent packages like lodash from working efficiently in the future we might get into trouble.

I would prefer to check some of our exported libs (React, Recharts, react-dnd, Angular...) and see if those are good examples of this problem.

[izaera]

I was thinking that, even if we put something in package.json and make imports automatic, we still have the possibility to do everything by hand (as before) in liferay-npm-bundler.config.js as we need something to override the automatism.

For people not wanting to use lerna, yarn, npm link, or a local registry that would suffice to configure the imports, as they would do it essentially the same way they do it now in bundler 2.

And for the rest of people (and especially us), well, life would be simplified a lot with this new automatic approach.

[jbalsas]

What I expect (and that's what we've seen until now in customer projects) is that providers and consumers are inside the same repo or next to each other. In fact, if someone has a setup where the providers are done by some other people than the consumers, they should consider installing a local npm registry, as that means that they are inside a project where responsibilities spread across different distributed teams and they need some way to integrate all together.

However, this is my assumption. I may be wrong because we don't see every project out there.

A key gap in this reasoning is external developers trying to consume our APIs or providers.

[wincent]

Otherwise, it would be impossible to read it from outer projects because you may need the whole source code to be able to execute liferay-npm-bundler.config.js which is something that will not be available in outer projects consuming the provider.

I am not sure I get this point. A bunch of other tools use or can use .js configuration files (Babel, Jest, ESLint, Prettier, webpack etc) without issue, so what exactly are you thinking could cause a problem here? As long as your config module exports a configuration object — and doesn't do anything crazy beyond a simple require, which should always work due to the Node module resolution algorithm (and none of the examples I just mentioned do anything crazy) — it is effectively statically analyzable, because you can just evaluate it.

Just so as to avoid a repeat of a previous discussion, I'll just link to my last comment on this from a few weeks back. I still don't see why we would need to support a truly dynamic config (ie. a config module that exports a function), and which would preclude us from resolving this statically.

[izaera]

@izaera: Otherwise, it would be impossible to read it from outer projects because you may need the whole source code to be able to execute liferay-npm-bundler.config.js which is something that will not be available in outer projects consuming the provider.

@wincent: I am not sure I get this point. A bunch of other tools use or can use .js configuration files (Babel, Jest, ESLint, Prettier, webpack etc) without issue, so what exactly are you thinking could cause a problem here? As long as your config module exports a configuration object — and doesn't do anything crazy beyond a simple require, which should always work due to the Node module resolution algorithm (and none of the examples I just mentioned do anything crazy) — it is effectively statically analyzable, because you can just evaluate it.

In this case I was not talking about reading the config file of the project you are working on, but the one from another project (that may be even published to npmjs.com). Configuring a project using a .js file is already merged and working without any problem :-).

Let's see what I mean with an example.

1. Imagine you create a provider for react (let it be frontend-js-react-web in our case) and then I want to use it in my project outside of the portal (because I'm deploying to Liferay and I want to use its react copy).

2. Now imagine you tweak the portal's build to rely on gradle to compute the bundler's configuration (this case may seem exaggerated, but in general, anything that you use to generate the bundler's configuration that isn't published to npmjs.com would have the same problem).

3. The end result is that I cannot read frontend-js-react-web's bundler configuration because I'm not able to execute liferay-npm-bundler.config.js. Thus, I cannot parse your exports to automatically import them in my project.

I'm not saying that we should move everything to .npmbundlerrc again but just move the exports to the package.json so that they can be easily read from anybody outside the project itself.

Also, putting things in package.json is the standard pattern used by mainstream bundlers (think of browser fields, for example) to override things, so it makes perfect sense to put exports there instead of liferay-npm-bundler.config.js, the same way we configure the portlet properties there or people specify alternate source files for bundlers.

[izaera]

Regarding:

@wincent: Just so as to avoid a repeat of a previous discussion, I'll just link to my last comment on this from a few weeks back. I still don't see why we would need to support a truly dynamic config (ie. a config module that exports a function), and which would preclude us from resolving this statically.

I've read your commit and noticed point 4 which I already implemented in this project, but made me think about it again.

The reason why I may need liferay-npm-bundler.config.js to export a function (currently I let it export an object or a function and I autodetect what it returned) is because the bundler creates an original webpack config that the user may want to override so I cannot think of any other way to pass it to the user.

However, I'm not yet sure if the user will need to override that config in the final image. In fact, that need is changing while I develop the solution which to me is a clear sign that we don't know yet. I'll add a TODO in the code to make sure we decide the correct thing before releasing and we will revisit it again to discuss the possibilities :+1: .

[izaera]

@izaera: What I expect (and that's what we've seen until now in customer projects) is that providers and consumers are inside the same repo or next to each other. In fact, if someone has a setup where the providers are done by some other people than the consumers, they should consider installing a local npm registry, as that means that they are inside a project where responsibilities spread across different distributed teams and they need some way to integrate all together.

However, this is my assumption. I may be wrong because we don't see every project out there.

@jbalsas: A key gap in this reasoning is external developers trying to consume our APIs or providers.

That's true. I see two solutions here:

1. It seems to me that an elegant solution would be publishing packages with the description of our modules. For example: we would publish a package named @liferay/frontend-js-react-web containing a package.json and any other files needed by the bundler to automatically detect what needs to be imported in a project in a zero configuration fashion.

However I'm unsure if we want to go that way now given that we need to setup a way to publish those packages automatically whenever we release a new Liferay version. But..., we don't need to take that decision now. We don't even need to publish every provider module we have in the portal, but just those that people use most or the ones we want to make generally available.

In general we can do it progressively and decide on the go which pattern is best, because we have solution 2 :point_down:

2. People wanting to consume our providers configure them the way we configured imports in bundler 2 (using liferay-npm-bundler.config.js and by hand). We will always have that possibility in our toolbox so we will never be in a worse position than in bundler 2. All we want to do now is making things easier to configure.

[wincent]

I'm not saying that we should move everything to .npmbundlerrc again but just move the exports to the package.json so that they can be easily read from anybody outside the project itself.

Is this what you are referring to in your other comment?

1. we would publish a package named @liferay/frontend-js-react-web containing a package.json and any other files needed by the bundler to automatically detect what needs to be imported in a project in a zero configuration fashion.

Because merely moving exports from frontend-js-react-web's bundler configuration file to its package.json won't address the issue that you highlight here:

3. The end result is that I cannot read frontend-js-react-web's bundler configuration because I'm not able to execute liferay-npm-bundler.config.js.

If you don't have local access and it's not published to NPM, then you can't read the bundler config file or the package.json.

[izaera]

This needs some more development because webpack supports ES6 imports out of the box and some packages out there (like recharts) provide transpiled source code with ES6 imports in place of requires.

This need arose from https://issues.liferay.com/browse/LPS-109921.