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

Specify Node 14 for the engines #387

Closed
wants to merge 1 commit into from
Closed

Conversation

jenweber
Copy link
Contributor

This is needed due to ember-auto-import's requirements around Node 14. Without this PR, someone using Node 12 would get a cryptic error message if they are using a lower version of Node. After this is merged, that same person would see the node engines warning when they start their server.

Closes #386

@mansona
Copy link
Member

mansona commented Apr 15, 2021

so... I'm a bit confused here 🤔 what exactly do you mean by ember-auto-import's requirements around Node 14? What was the cryptic error message that you were seeing?

I would be super worried if this is telling us that we need to use Node 14 for styleguide because that doesn't align with Ember and it would mean we need to do a major bump here to account for it. Also if this is the truth then we would probably need to upgrade our CI config too 🤔

I notice that you were committing a package-lock that was downgrading the lockfileVersion (meaning that you are using npm@6 locally). Maybe this was causing your issue?

@jenweber
Copy link
Contributor Author

The error is described in the ticket. It is:

/var/folders/qd/tw7r5_211zq0jqwkpf9ry6gw0000gn/T/broccoli-83033cSaYf6UYBtoo/out-359-append_ember_auto_import_analyzer/assets/vendor.js:24038
      this.forEach(item => ret.push(item[methodName]?.(...args)));
                                                     ^

SyntaxError: Unexpected token '.'
    at new Script (vm.js:88:7)

The ?. syntax is optional chaining, which supported in Node >=14

Reproduction:

  • Use node 12 (i.e. nvm use 12 and node --version to verify)
  • Start the server (again making sure you are on node 12)
  • Visit localhost:4200

The problem emerges only when you visit the page. The app builds fine.

You can see that ember-auto-import specifies that volta should use Node 14: https://github.com/ef4/ember-auto-import/blob/master/package.json#L28

@jenweber
Copy link
Contributor Author

Also, I updated the commit to fix the npm version issue.

@jenweber
Copy link
Contributor Author

Closed in favor of #390

@jenweber jenweber closed this Apr 16, 2021
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.

Update Node engine version
2 participants