Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add custom browserslist #2290

Merged
merged 7 commits into from
Nov 14, 2024
Merged

chore: Add custom browserslist #2290

merged 7 commits into from
Nov 14, 2024

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Nov 7, 2024

Now that we have an idea what browsers our vendors use, we can add a browserslist config to help catch incompatibilities. I added this to ESLint and StyleLint so CI can help us find issues.


@cmaddox5 cmaddox5 requested a review from a team as a code owner November 7, 2024 16:19
assets/.browserslistrc Show resolved Hide resolved
assets/.browserslistrc Outdated Show resolved Hide resolved
@@ -38,5 +39,9 @@ export default {
// Conflicts with Prettier, which breaks long lines involving operators by
// putting operators at the ends of lines.
"scss/operator-no-newline-after": null,
"plugin/no-unsupported-browser-features": [
true,
{ ignore: ["intrinsic-width"] },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we ignore something we should probably add a note explaining why. I assume it's because the only places we use this feature are on screen types that actually do support intrinsic-width?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before adding the correct Bus E-Ink version, this was the only error we needed to deal with and the browser it is an issue for doesn't reach the particular CSS in question. After the update, there's a 919 errors so adding global ignores isn't a good idea. I'm curious how best to handle this. At a webpack level, that seems as easy as separating out the apps into their own entries and specifically calling out the browser each entry will use. I'm not sure about the linters though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... is this warning us about using features that Babel will ultimately compile down to something that does actually work? Or are those warnings truly about features that can't be supported automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter. And they tell us which particular browser it has an issue on. That's how I know the errors don't apply to the particular browser running the code. So for example, after adding Chrome 45 (Bus E-Ink) and Firefox 90 (Sectional) we get errors in Pre-Fare specific CSS files. I believe these checks are also happening with no knowledge of the babel config so I'm not sure if it's possible to make it work on a per-screen type basis. Gonna do some more reading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up question for investigation, does the Webpack build itself throw an error or warning when we use a feature Babel can't transpile to the target browsers, or does it silently allow such features?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, if these are CSS issues then I suppose Babel wouldn't be doing anything about them. We'd need something like postcss-preset-env in our pipeline to transpile CSS features.

I wonder if we should split stylesheet compatibility into a separate task here. We're already pretty sure there are no issues with the CSS we have now, so it perhaps shouldn't be considered a requirement of this task, which is getting us better JS compatibility so we can upgrade libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds good. Will remove those and create a task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if we should do the same with ESLint. The lint errors showing up right now are either false positives or handled by the polyfills we are already importing that ESLint has no knowledge of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I would have expected the opposite based on this thread, which warns that the ESLint plugin is too lenient in assuming which polyfills are available (suggesting there is some interaction with Babel).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have just configured the plugin incorrectly (documentation is lackluster unfortunately). I made an adjustment and the errors are gone. I found in the source code for the plugin that it assumes polyfills are done in babel based on the presence of a babel config file. We didn't have one so I made it. Hopefully this doesn't mean I just silenced the errors by accident.

@digitalcora
Copy link
Contributor

I don't see a change to our Webpack config file here; we have this in there:

["@babel/preset-env", { targets: "> 0.25%" }]

I assume we can now at least remove the targets configuration here and let browserslist take care of it — does present-env read that by default?

@cmaddox5
Copy link
Contributor Author

cmaddox5 commented Nov 7, 2024

I don't see a change to our Webpack config file here; we have this in there:

["@babel/preset-env", { targets: "> 0.25%" }]

I assume we can now at least remove the targets configuration here and let browserslist take care of it — does present-env read that by default?

Woops, thank you. And yes, preset-env would pick it up by default.

assets/.browserslistrc Outdated Show resolved Hide resolved
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment. I do like how it's possible to pull out the Babel config like this, simplifies the main Webpack config structure.

@@ -0,0 +1,16 @@
{
"presets": [
// When no targets are specified: Babel will assume you are targeting the oldest browsers possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invalid JSON, though I guess Babel allows it? "Oldest browsers possible" def isn't accurate with how this is set up, so I think we should delete.

@cmaddox5 cmaddox5 merged commit f5abc2e into main Nov 14, 2024
11 checks passed
@cmaddox5 cmaddox5 deleted the cm/add-browserslist branch November 14, 2024 18:35
cmaddox5 added a commit that referenced this pull request Nov 22, 2024
cmaddox5 added a commit that referenced this pull request Nov 22, 2024
cmaddox5 added a commit that referenced this pull request Nov 26, 2024
cmaddox5 added a commit that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants