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

Use getNodeInfo() to get plugin/node name/label" #29

Merged

Conversation

oligriffiths
Copy link
Contributor

@oligriffiths oligriffiths commented May 23, 2018

Reverts #28

I need to work on testing instruction using npm link or something to test with a new ember app

@oligriffiths oligriffiths changed the title Revert "Revert "Use __broccoliGetInfo__() to get plugin/node name/label"" [WIP] Revert "Revert "Use __broccoliGetInfo__() to get plugin/node name/label"" May 23, 2018
package.json Outdated
@@ -30,6 +30,7 @@
"silent-error": "^1.0.1"
},
"devDependencies": {
"broccoli-node-info": "^1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Will need to be a dep not a devDep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, yes, oops

@@ -296,6 +306,14 @@ function getPluginName(tree) {

exports.getDescription = getDescription
function getDescription (tree) {
if (typeof tree !== 'string') {
try {
Copy link
Member

Choose a reason for hiding this comment

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

What is this protecting us against? I don’t think we want to swallow any/all errors here.

I think the swap to broccoli-node-info should address the originally reported issue properly without the try / catch...

Copy link
Contributor Author

@oligriffiths oligriffiths May 23, 2018

Choose a reason for hiding this comment

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

https://github.com/broccolijs/broccoli-node-info

This function throws a broccoliNodeInfo.InvalidNodeError when called with

  • a plain string node (while still supported by the Broccoli Builder, they are deprecated in favor of broccoli-source),
  • a node using the old .read/.rebuild API (see docs/broccoli-1-0-plugin-api.md), or
  • some other object that isn't a node.

Seem like 2 could be a problem

Copy link
Member

Choose a reason for hiding this comment

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

ya, sounds good, lets check for the specific error and only swallow that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

lib/builder.js Outdated
@@ -4,6 +4,7 @@ var apiCompat = require('./api_compat')
var heimdall = require('heimdalljs');
var SilentError = require('silent-error');
var BroccoliBuildError = require('./broccoli-build-error');
var BroccoliNodeInfo = require('broccoli-node-info');
Copy link
Member

Choose a reason for hiding this comment

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

Can you do this instead:

var getNodeInfo = require(‘broccoli-node-info’).getNodeInfo;

Seems like it would make the call sites simpler...

@oligriffiths oligriffiths changed the title [WIP] Revert "Revert "Use __broccoliGetInfo__() to get plugin/node name/label"" Use getNodeInfo() to get plugin/node name/label" May 23, 2018
@oligriffiths
Copy link
Contributor Author

So, I've tried this with:
yarn link (in broccoli-builder
npm install -g ember-cli@3.1.3
ember new ember-test
yarn link broccoli-builder
ember b
All fine.

Also switched to the "broken" branch, and it works just fine there too. Soooo, not sure.

@rwjblue rwjblue merged commit 9bf64e4 into ember-cli:0-18-x May 25, 2018
@oligriffiths oligriffiths deleted the revert-28-revert-27-plugin-name branch May 28, 2018 20:28
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.

None yet

2 participants