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

doc: Mention --experimental-top-level-await flag #33473

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 19, 2020
@juanarbol
Copy link
Member

I don't want to be against this change, but is it currently documented here

@dfabulich
Copy link
Contributor Author

I didn't mean to say that it's not documented, but the changelog should mention that it's behind the flag, I think.

@devsnek
Copy link
Member

devsnek commented May 20, 2020

@dfabulich
Copy link
Contributor Author

I filed a separate PR for nodejs.org.

@samceena
Copy link

The changelog does not mention that it's behind a flag if it needs to be, so it's confusing.
If it try: node --harmony-top-level-await 1.mjs it works, but if i try node 1.mjs, its not working, even if i have "type": "module" in the package.json file, so it's a little ambiguous. Can we write more about this in the doc?

samceena
samceena approved these changes May 20, 2020
@zombieleet
Copy link
Contributor

@dfabulich it is also not specified in the cli doc https://nodejs.org/api/cli.html

@dfabulich
Copy link
Contributor Author

Good catch! I added documentation to cli.md.

@dfabulich
Copy link
Contributor Author

While updating cli.md, I briefly incorrectly claimed that --experimental-top-level-await works in NODE_OPTIONS, but then I realized it doesn't, so I filed PR #33495 to add it.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2020
@dfabulich
Copy link
Contributor Author

These tests were passing before I added that whitespace change; I'm about 99% sure this is a flaky build issue. (I don't see any way to rerun the build myself.)

@dfabulich dfabulich force-pushed the patch-1 branch 2 times, most recently from 26ca8c3 to dfaa01b Compare May 22, 2020 04:11
@dfabulich
Copy link
Contributor Author

I'm about 99.999% sure this documentation PR didn't cause this failing test.

not ok 844 parallel/test-http-destroyed-socket-write2
  ---
  duration_ms: 0.125
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:103
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + 'ERR_STREAM_DESTROYED'
    - 'ECONNRESET'
        at ClientRequest.<anonymous> (/Users/runner/runners/2.262.1/work/node/node/node-v15.0.0-nightly2020-05-22xxxx/test/parallel/test-http-destroyed-socket-write2.js:66:16)
        at ClientRequest.<anonymous> (/Users/runner/runners/2.262.1/work/node/node/node-v15.0.0-nightly2020-05-22xxxx/test/common/index.js:363:15)
        at ClientRequest.emit (events.js:315:20)
        at emitErrorNt (_http_outgoing.js:666:43)
        at processTicksAndRejections (internal/process/task_queues.js:85:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_STREAM_DESTROYED',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

@BridgeAR
Copy link
Member

@dfabulich we have multiple flaky tests. It is some times difficult to get the tests passing consistently on the amount of different systems and settings we support.

If you stumble upon one of those, you might want to check if there's a corresponding issue and if not, to open a new issue for the failure.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 23, 2020
PR-URL: nodejs#33473
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

Landed in c39467c 🎉

@BridgeAR BridgeAR closed this May 23, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
PR-URL: #33473
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
PR-URL: #33473
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #33473
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants