Lint against explicit use of default npm bundler preset in liferay portal

[wincent]

Just like we did in https://github.com/liferay/eslint-config-liferay/issues/130 to stop redundant instances of @babel/preset-react/@babel/preset-env in liferay-portal, we should do the same for .npmbundlerrc and the default preset ("liferay-npm-bundler-preset-liferay-dev"), which applies — unless configured otherwise — wherever we use the bundler via this config file in liferay-npm-scripts.

As was the case with .babelrc and .eslintrc, we need to force the usage of .npmbundlerrc.js instead of just .npmbundlerc (JSON) so that ESLint can actually process the config. If we do this, we get this and any other linting/formatting "for free". I don't know whether the bundler is currently equipped to read ".js" files though, so changes may be needed there in order for this to happen.

[wincent]

@izaera: can the bundler currently leverage .npmbundlerrc.js as a config file format or will we need changes there too?

[izaera]

@wincent .npmbundlerrc.js support is not yet available. I wanted to add it long ago but it was not so easy, so I decided to leave it for bundler v3.

[wincent]

Just echoing what I [said here]():

It may be quite easy if you use cosmiconfig. It's already in the liferay-js-toolkit dependency graph, so you could make that explicit and rely on that:

=> Found "cosmiconfig@5.2.1"
info Reasons this module exists
   - "_project_#lerna#@lerna#list#@lerna#command#@lerna#project" depends on it
   - Hoisted from "_project_#lerna#@lerna#list#@lerna#command#@lerna#project#cosmiconfig"

We're using it in liferay-npm-tools also.

[izaera]

Not quite sure, because we don't want a JS function that returns a static object, but something more complex. Some of the configuration in .npmbundlerrc is pure strings, but some other is loaders, plugins, and the like, that should be returned as instantiated functions by the .npmbundlerrc.js instead of as strings and the code managing the configuration in the bundler is not prepared for that right now.

I'm not saying that it is too difficult, but definitely not direct. Also, I think we should take advantage of that implementation to enhance the format a bit to ease configuration merging, for example.

Nevertheless, .npmbundlerrc is pure JSON. Can't we format it with eslint? :thinking:

[wincent]

Not quite sure, because we don't want a JS function that returns a static object, but something more complex.

I'm not convinced about that. Do we have an actual use case that would necessitate that complexity?

Pretty much all of the tools that support JS configuration files expect (or at least allow) them to export an object that corresponds to what you would write in JSON if you were doing it statically. Babel, for example, allows you to export a dumb object (which you can of course create using whatever arbitrary logic you want), or to export a function that will be passed a config API. In liferay-portal, we only ever export dumb objects, precisely to facilitate configuration merging like you mention.

As counter examples to Babel, both ESLint and Prettier just expect dumb objects. I've yet to see a case where the Babel function-export approach was actually necessary.

Nevertheless, .npmbundlerrc is pure JSON. Can't we format it with eslint? 🤔

We can likely configure Prettier to read this file and tell it that it actually contains JSON, but it is much easier to do so when we can rely on a file extension. (This is probably why tools like Babel encourage people to use config files with explicit file extensions, even though it can still read .babelrc for legacy compatibility reasons.) Prettier won't help us with sorting, however, only formatting.

Not sure about ESLint though; AFAIK it only wants to deal with JavaScript source, and while JSON is JavaScript, it's probably using the wrong tool for the job.

[izaera]

I'm not convinced about that. Do we have an actual use case that would necessitate that complexity?

I was thinking in creating loaders on the fly inside the very .npmbundlerrc.js file, for example. If you cannot return functions, you would need to put them on their own .js file and reference them by absolute path to make sure they get loaded. Also, you wouldn't be able to make use of closures and would need to pass all explicit configuration as a static configuration object.

It's not so dramatic, but it would be great to be able to declare loaders on the fly, as you can do in webpack, for example. It leads to a much simpler configuration and lets the developer make little tweaks with ease.

We suffered a similar limitation when we switched from packages to modules to specify loaders and plugins in the .npmbundlerrc file and the change was quite positive. In this case it would be even better.

[izaera]

In any case, I'm not saying that we should go one way or the other. Simply, that it deserves a bit of analysis to decide what to do.

[izaera]

If this is needed we can go with the first approach in bundler 2 and rethink it in bundler 3. In any case we won't lose anything when supporting .js files returning plain objects.

[wincent]

I was thinking in creating loaders on the fly inside the very .npmbundlerrc.js file, for example. If you cannot return functions, you would need to put them on their own .js file and reference them by absolute path to make sure they get loaded.

It's not so dramatic, but it would be great to be able to declare loaders on the fly, as you can do in webpack, for example. It leads to a much simpler configuration and lets the developer make little tweaks with ease.

I think you might be conflating two orthogonal things here:

1. Whether the config file should export an object (as opposed to a function).
2. Whether you can use functions as properties within that object.

Even webpack, which is incredibly dynamic, expects its config files to export objects. I don't see any reason why the bundler couldn't allow people to define loaders inline as functions if we decide we want them to (still not clear to me that it would be necessary though, but I think that's a separate topic).

In other words, there is a ladder of complexity here that runs like this:

1. Config if a dumb JSON file.
   • This is the level that the bundler currently uses.
2. Config is a JS module that returns an object:
   • The code in that JS module is arbitrary JS and so can compute anything that JS can compute.
   • This is the level that we currently use in other config files in liferay-portal, and is also the level that corresponds to your example of "generating an absolute path" for later reference.
3. Config is a JS module that returns an object, and we allow properties in that object to be dynamic (ie. functions instead of static values):
   • This is the level that webpack uses.
4. Config may be a JS module that returns a function:
   • Function receives an API object as a parameter, effectively allowing inversion of control.
   • This is the level that Babel (optionally) uses, and which I haven't seen a convincing case for yet.

I don't think we need to race all the way to level 4 without clear cause.

[bryceosterhaus]

Gonna close as we don't have the capacity for it. Will re-open in future if necessary