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.

Sign up for GitHub

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

Jump to bottom

IE11 compatibility for top-level await #18487

Closed
wants to merge 4 commits into from
Closed

IE11 compatibility for top-level await #18487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e842769
Add failing test.
amannn Oct 29, 2020
3d972c2
Fix formatting
amannn Oct 29, 2020
faf1f20
Try modifying the root package.json
amannn Oct 29, 2020
e274650
Fix Syntax
amannn Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/build_test_deploy.yml
View file
Open in desktop
Expand Up @@ -107,9 +107,9 @@ jobs:
steps:
- uses: actions/checkout@v2
- run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*
- run: cat packages/next/package.json | jq '.resolutions.webpack = "^5.0.0-beta.30"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat packages/next/package.json | jq '.resolutions.react = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat packages/next/package.json | jq '.resolutions."react-dom" = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat package.json | jq '.resolutions.webpack = "^5.0.0-beta.30"' > package.json.tmp && mv package.json.tmp package.json
- run: cat package.json | jq '.resolutions.react = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp package.json
- run: cat package.json | jq '.resolutions."react-dom" = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp package.json
Copy link
Contributor Author

@amannn amannn Oct 29, 2020 •
edited

Choose a reason for hiding this comment

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

I noticed that the webpack 5 tests never ran, as webpack remained at 4.x on CI. When #17590 was merged, it looks like all tests were simply skipped:

Running tests: 
 test/integration/async-modules/test/index.test.js

  console.log test/jest-setup-after-env.js:5
    Configuring jest retries: 0

Test Suites: 1 skipped, 0 of 1 total
Tests:       24 skipped, 24 total
Snapshots:   0 total
Time:        4.269s

From https://github.com/vercel/next.js/runs/1252336307

With the change above, they run as expected. Does this look ok to you or am I overlooking something?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 interesting. That means none of the webpack 5 tests are currently actually testing webpack 5 integration?

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 not sure, I'm really new to the repository and not familiar with the general setup. I just noticed that my added test didn't fail on CI and therefore I've adjusted this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw. the project where I originally found this bug is using the currently latest version of webpack: 5.3.1.

Copy link
Contributor

@Janpot Janpot Oct 29, 2020 •
edited

Choose a reason for hiding this comment

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

And this doesn't happen on next 10 + webpack 4?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, does the bundle contain async () => in webpack 4, even when not using top-level-await?

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 haven't checked that yet, but what I did try is using webpack 5 and using async/await in the regular component code in _app. These statements were changed to ES5 as expected, but only the top-level await code resulted in native async/await code.

That made me wondering if top-level await only works with native async/await but I'd assume there should be some way for webpack to resolve the modules with promises and calling then.

As far as I understand the build chain, the user-land async/await code is compiled with Babel and then provided to webpack. But for top-level await code Babel doesn't attempt to transform it, since it needs to rely on the bundler to do this. The bundler chooses to turn it into native async/await which happens to break in IE11.

So I'm wondering if this is really a webpack issue, as I guess you wouldn't want to run babel again on the bundle code that webpack has produced.

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've created a minimal reproduction only with webpack now and opened an issue in the webpack repository to discuss this: webpack/webpack#11874.

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi, I also included these changes in #18589

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks!

- run: yarn install --check-files
- run: node run-tests.js test/integration/production/test/index.test.js
- run: node run-tests.js test/integration/basic/test/index.test.js
Expand Down
13 changes: 13 additions & 0 deletions test/integration/async-modules/test/index.test.js
View file
Open in desktop
Expand Up @@ -101,6 +101,19 @@ function runTests(dev = false) {
if (browser) await browser.close()
}
})
;(dev ? it.skip : it)('compiles the source to es5', async () => {
const html = await renderViaHTTP(appPort, '/')

const appScriptMatch = html.match(
/\/_next\/static\/chunks\/pages\/_app-.+?\.js/
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there's a way to avoid hardcoding this path or to generally rewrite this test. For now I only tried to get a failing test that demonstrates the issue.

expect(appScriptMatch).toBeTruthy()

const appScript = appScriptMatch[0]
const appScriptContent = await renderViaHTTP(appPort, appScript)
expect(appScriptContent).not.toMatch(/async\(\)=>/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails currently.

expect(appScriptContent).not.toMatch(/await\s/)
})
}

;(isWebpack5 ? describe : describe.skip)('Async modules', () => {
Expand Down