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

Image config from server (not transpiled) being imported in client #34629

Closed
1 task done
vitorhsb opened this issue Feb 21, 2022 · 8 comments · Fixed by #34677
Closed
1 task done

Image config from server (not transpiled) being imported in client #34629

vitorhsb opened this issue Feb 21, 2022 · 8 comments · Fixed by #34677
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@vitorhsb
Copy link
Contributor

vitorhsb commented Feb 21, 2022

Verify canary release

  • I verified that the issue exists in Next.js canary release

Provide environment information

    Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_X86_64
    Binaries:
      Node: 14.17.6
      npm: 6.14.15
      Yarn: 1.22.17
      pnpm: N/A
    Relevant packages:
      next: 12.1.1-canary.1
      react: 17.0.2
      react-dom: 17.0.2

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

The file server/image-config is being imported in next/dist/client/image.js.
This injects non transpiled content into the main chunk file which in turn makes the client build not safe for ES5 and not working in IE11.

Expected Behavior

Only transpiled to ES5 (IE11) content in client builds.

To Reproduce

Create a new project from scratch:

npx create-next-app --example image-component image-app
cd image-app
npx --no-install next build

Run es-check against .next folder:

npx es-check es5 '.next/static/**/*.js' -v

The following error appears:

error: ES-Check: there were 1 ES version matching errors.

info:
          ES-Check Error:
          ----
          · erroring file: .next/static/chunks/main-01df828e572375b9.js
          · error: SyntaxError: The keyword 'const' is reserved (1:99549)
          · see the printed err.stack below for context
          ----

          SyntaxError: The keyword 'const' is reserved (1:99549)
@vitorhsb vitorhsb added the bug Issue was opened via the bug report template. label Feb 21, 2022
@balazsorban44
Copy link
Member

Hi, const is supported in IE11, and technically it's not ES5 compatibility we provide, but IE11 support.

https://caniuse.com/?search=const

https://nextjs.org/docs/basic-features/supported-browsers-features

Do you have a reproduction that breaks in IE11 because of this?

@vitorhsb
Copy link
Contributor Author

Hi @balazsorban44 , thanks for your feedback.

According to https://caniuse.com/?search=const it seems that const is only partially supported, according to the note "Not supported in for-in and for-of loops". Which means that every client code which uses const and/or let (partial support) are being transpiled back to ES5 variant.

So it seems pretty odd that from a recent release (we have recently updated to 12.1.0) server code is being included in the client build as not transpiled.
To be clear, it does not break anything for now for IE11 but I think it is an anti-pattern and could leverage some problems in the future. I believe that, if some files are being imported in both client and server they should be extracted to a common place.

We have been using es-check es5 on CI steps to make sure a node dependency does not break anything, and if some dependency fails to support es5 it is being added with next-transpile-modules.
If you decide not to create an es5 safe build, then we may have to blindly remove this step.

@balazsorban44
Copy link
Member

Thanks for the extra context. 👍

The imported server file exports a single object and is not used in any for loops, so it really should be safe:

export const imageConfigDefault: ImageConfigComplete = {
deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 3840],
imageSizes: [16, 32, 48, 64, 96, 128, 256, 384],
path: '/_next/image',
loader: 'default',
domains: [],
disableStaticImages: false,
minimumCacheTTL: 60,
formats: ['image/webp'],
dangerouslyAllowSVG: false,
contentSecurityPolicy: `script-src 'none'; frame-src 'none'; sandbox;`,
}

could leverage some problems in the future

I'm curious if you have an example. I think the industry is moving away from IE11 (which as mentioned isn't even an issue here), so it's less likely that this could introduce an issue in the future.

If you target IE11 users specifically, you could let the CI check test for IE11 features, instead of ES5, as seemingly there is no 100% overlap

In reality though, if something would break in IE11 and is reported as a bug in Next.js, we would obviously release a fix immediately, as we do intend to support IE11 currently, and even in the future, we will let it be an opt-in target. See our RFC: #33227

That said, I agree it does not hurt to do what you propose, so I'll leave this issue open as a potential improvement opportunity. We should likely move the file as you suggested from the server folder and make sure it's included uniformly in the transpilation process.

@balazsorban44 balazsorban44 added area: Reliability good first issue Easy to fix issues, good for newcomers and removed bug Issue was opened via the bug report template. labels Feb 21, 2022
@vitorhsb
Copy link
Contributor Author

As mentioned in the RFC #33227, "Next.js currently targets ES5 for JS output". This was somehow my initial presumption to create this kind of issue.

For the moment we have turned our es-check CI step from an error to a warning and will manually check for validation errors. We also have a simple e2e test targeting IE11 to check minimal functionality by loading login page and login action.

Does Next.js have e2e tests agains IE11?

@Phryxia
Copy link

Phryxia commented Feb 22, 2022

Hi, @balazsorban44 ! Recently I've been working on migration to Next 12 at my company.

At first I'd like to follow up #33854 in to my project, so I installed 12.1.1-canary.1 and I got stucked with this issue.
I'd like to provide more context for this.

Do you have a reproduction that breaks in IE11 because of this?

Yes, IE 11 emits "Invalid Character" error with the white screen. As @vitorhsb mentioned, this is because a server side code (dist/server/image-config.js) was included. Since the error doesn't tell the exact position but 1, I'll show the transpiled one below.

/***/ "./node_modules/next/dist/server/image-config.js":
/*!*******************************************************!*\
  !*** ./node_modules/next/dist/server/image-config.js ***!
  \*******************************************************/
/***/ (function(__unused_webpack_module, exports) {

"use strict";
// Here!!!! // eval("\nObject.defineProperty(exports, \"__esModule\", ({\n    value: true\n}));\nexports.imageConfigDefault = exports.VALID_LOADERS = void 0;\nconst VALID_LOADERS = [\n    'default',\n    'imgix',\n    'cloudinary',\n    'akamai',\n    'custom', \n];\nexports.VALID_LOADERS = VALID_LOADERS; ...

I hope this might help. Can I get any hint for fixing this (either at Next.js or my project)? If it's not that hard, maybe I might try to fix this for my good first issue 😅.

And is there any other ways to overcome #33854 without using 12.1.1-canary.1? (This is done on Centos7)

@balazsorban44
Copy link
Member

As mentioned in the RFC

Good catch, let's fix this then! 🙂

Yes, IE 11 emits "Invalid Character"

I tried to reproduce it, but I could not. Here is a deployment using an image with next/image but it works fine in IE11. https://balazsorban-34629.vercel.app

Could you attach a reproduction?

And is there any other way to overcome ...

That is an unrelated issue, so let's keep this one to the point, but no, you currently either have to upgrade to canary or wait for a patch release on latest.

@balazsorban44
Copy link
Member

balazsorban44 commented Feb 22, 2022

I've opened a PR to fix this and I can confirm it will let es-check pass again for ES5:

image

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants