openedx / browserslist-config

GNU Affero General Public License v3.0
3 stars 3 forks source link

We should include node versions too #6

Open mikix opened 2 years ago

mikix commented 2 years ago

Without specifying a node version (that is, by having an implicit dependency on whatever node version openedx is currently building on), we can easily break builds by using new JS features that came out after the node we use (currently 12, soon to be 16).

For example, we could use some fancy new JS feature that babel provides a polyfill for via the '2 last firefox' line. But then firefox gains support for it and babel stops polyfilling it. Then our CI tests suddenly and curiously break because node16 never supported it and chokes on the new syntax.

Let's make our node reliance explicit!

This sounds like it could maybe be a one-liner, like adding current node to our list. But this was attempted before and then reverted because it broke our builds.

I don't know the specific error that occurred, but I do know that webpack will error out if you don't also set webpack.target = 'web' because webpack can no longer infer that. Maybe other tooling would get also surprised by the addition of a node version in the browserslist.

So this ticket is to keep track of the need in general for adding node versions to the browserslist and maybe collect the work items that we'd need to fix when we do make this change.

The only one I know of now is also adding target = 'web' to frontend-build. But there ought to be some discovery work to suss out if that's it or there is more work needed.

davidjoy commented 2 years ago

I'm a bit confused, are we saying there's code that runs in node - i.e., as part of the webpack process - that is subject to browserslist? I'd expect that our webpack config and the loaders/plugins we rely on should have support for a set of node versions, and that so long as we're using a node version that all those loaders/plugins support our build should succeed regardless of what's in browserslist. And that browserslist is just determining the set of polyfills and transformations being applied to our built code artifacts.

Is it the case that our node envirionment needs to have a superset of features of whatever browsers we're supporting because babel needs to be able to read our source code in a node environment?

It feels odd to include node in our browserslist in spirit, since we don't expect nor want to support our transpiled application code being run in a node environment. We expect our source code to, which isn't what browserslist is there to specify.

If all the above is true, is the answer that we don't write code in our source that our node version doesn't support, and then leave browserslist as-is?

mikix commented 2 years ago

I'm a bit confused, are we saying there's code that runs in node - i.e., as part of the webpack process - that is subject to browserslist?

This is based on second-hand reports for me. I was told that one reason we couldn't drop IE11 from some MFE browserslist configs is that node12 would lose some polyfills it needed and CI would fail. This is from @adamstankiewicz, but I'm not sure which MFEs are affected or what the errors were - there was not a ticket for that issue, so I made this.

Based on his report, I had assumed that our jest tests transpiled the source code based on the browserslist config. But is that not how things work (or at least, not how they should work)?

If all the above is true, is the answer that we don't write code in our source that our node version doesn't support, and then leave browserslist as-is?

Ah, I had been working under the assumption that (A) jest used transpiled tests & source based on the browserslist config and (B) we'd rather have unnecessary polyfills in our release artifacts than restrict our available JS features to what the current CI node version supports. But if A is false or folks prefer the other side of the tradeoff in B, then I guess this ticket could be closed.

Another solution here would be to generate two different artifacts - one targeted at the current node version for CI's benefit, and one for prod with just the supported browsers. But that seems bad, to not be testing on the same config used for prod.