Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

esm: remove support for arrays in import internal method #48296

Merged
merged 2 commits into from
Jun 5, 2023
Merged

esm: remove support for arrays in import internal method #48296

merged 2 commits into from
Jun 5, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 2, 2023 •
edited
Loading

This avoids initializing arrays that we never use, and simplifies the implementation overall.

This had a purpose before #43772, but now we load everything serially making the array creation superfluous.

@aduh95
esm: remove support for arrays in import internal method
1ab810b 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jun 2, 2023
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 2, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52057/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52058/

jlenon7
jlenon7 approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@jlenon7 jlenon7 left a comment

Choose a reason for hiding this comment

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

Nice, much simpler to read the code.

@jlenon7 jlenon7 mentioned this pull request Jun 2, 2023
Merged
4 tasks
JakobJingleheimer
JakobJingleheimer approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Good catch 🙂

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52059/

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 2, 2023 •
edited
Loading

Failures that seems to not be a flake, yet I don't see how it relates to this PR 🤔

not ok 106 parallel/test-child-process-pipe-dataflow
  ---
  duration_ms: 221.12800
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:125
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + '130086'
    - '1048577'
        at process.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-child-process-pipe-dataflow.js:71:12)
        at process.emit (node:events:523:35) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: '130086',
      expected: '1048577',
      operator: 'strictEqual'
    }
    
    Node.js v21.0.0-pre
  ...

EDIT: no it is a flake, it's failing on other jobs as well, taking it to its own issue.

@aduh95 aduh95 mentioned this pull request Jun 2, 2023
Open
@github-actions github-actions bot mentioned this pull request Jun 3, 2023
Open
33 tasks
@github-actions github-actions bot mentioned this pull request Jun 4, 2023
Open
30 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52092/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52096/

@github-actions github-actions bot mentioned this pull request Jun 5, 2023
Open
30 tasks
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9f3466b into nodejs:main Jun 5, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9f3466b

@aduh95 aduh95 deleted the fewer-arrays-import branch June 5, 2023 08:59
targos pushed a commit that referenced this pull request Jun 5, 2023
@aduh95 @targos
esm: remove support for arrays in import internal method
11a3434 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: #48296
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jun 5, 2023
Merged
This was referenced Jun 6, 2023
Open
Open
Open
@aduh95 aduh95 mentioned this pull request Jun 11, 2023
Merged
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jun 30, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@aduh95 @Ceres6
esm: remove support for arrays in import internal method
db25dc6 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: nodejs#48296
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@aduh95 @Ceres6
esm: remove support for arrays in import internal method
71715e4 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: nodejs#48296
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
@aduh95 @targos
esm: remove support for arrays in import internal method
96edd85 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: nodejs#48296
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Nov 11, 2023
Closed
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
@aduh95 @targos
esm: remove support for arrays in import internal method
1c7be60 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: #48296
Backport-PR-URL: #50669
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
esm: remove support for arrays in import internal method
0fe2237 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: nodejs/node#48296
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
esm: remove support for arrays in import internal method
0845e6e 
This avoids initializing arrays that we never use, and simplifies the
implementation overall.

PR-URL: nodejs/node#48296
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ljharb ljharb ljharb left review comments

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@jasnell jasnell jasnell approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@jlenon7 jlenon7 jlenon7 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@aduh95 @nodejs-github-bot @ljharb @jasnell @GeoffreyBooth @benjamingr @targos @JakobJingleheimer @jlenon7 @danielleadams