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

Ensure styles tree has been processed before checking its outputPath #239

Closed
wants to merge 1 commit into from

Conversation

rwjblue
Copy link

@rwjblue rwjblue commented Jun 10, 2021

Prior to this change, the ModuleSourceFunnel would assume that the passed in stylesTree has been processed when its build hook was called. This happened to work in some cases, but since Broccoli builder uses a DAG to decide what order to build trees in it absolutely could find a different sort order that causes ModuleSourceFunnel to run before that styles tree.

As part of this, I updated broccoli-funnel to allow supering with either a single node (prior behavior) or an array (broccolijs/broccoli-funnel#146). broccoli-funnel itself will never look at ainputs other than inputPath[0] but this is useful to ensure the broccoli node graph indicates the required ordering.


This fixes an error in Embroider builds where ember-css-modules is a child of an engine. The error you might get in that scenario is:

Build Error (OneShot)

BroccoliPlugin: this.outputPath is only accessible once the build has begun.
        at ModuleSourceFunnel

Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

Thanks @rwjblue! There are any number of questionable reaching-across-boundaries situations like this in ECM, so it's nice to at least have this one shored up 🙂

I may be mis-diagnosing, but with your change in broccolijs/broccoli-funnel#146 it looks like passing additional trees to Funnel's constructor may have more of an impact than just affecting the shape of the node graph, since this.input in the plugin now contains additional files. Any thoughts there?

@@ -223,8 +223,7 @@ function formatJS(classMapping) {

class ModuleSourceFunnel extends Funnel {
constructor(input, stylesTree, options) {
super(input, options);
this.stylesTree = stylesTree;
super([input, stylesTree], options);
Copy link
Member

Choose a reason for hiding this comment

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

Just based on my own understanding, I would have expected the change you made to Funnel to work exactly how you described, but it looks like this does end up having an effect on the base class's behavior.

I haven't dived super deep, but I'm seeing contents from both trees show up in this.input.entries(...), which seems to cause the funnel to try and lstat an ultimately nonexistent file from the second input tree.

Copy link
Author

Choose a reason for hiding this comment

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

Ya, this is definitely true. I replicated that specific error and fixed it over in broccolijs/broccoli-funnel#147.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated this PR with that change, which should avoid the error we see in CI.

@@ -72,7 +72,7 @@
"broccoli-bridge": "^1.0.0",
"broccoli-concat": "^3.2.2",
"broccoli-css-modules": "^0.7.0 || ^0.8.0",
"broccoli-funnel": "^2.0.1",
"broccoli-funnel": "^3.0.7",
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 ponder a bit what this means for semver, since ECM hasn't cut a major since Node 6 was still supported. Pragmatically Node 6 and 8 are both more than a year out of maintenance support themselves and I hope no one is using them, but letter of the law dropping support for them should probably mean a major bump here, which would be a bit of a pain.

Copy link
Author

Choose a reason for hiding this comment

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

Ya, sorry about that. I might be able to backport these changes to the 2.x branch of broccoli-funnel, but I don't even know if our CI works that far back at this point.

Prior to this change, the `ModuleSourceFunnel` would assume that the
passed in `stylesTree` has been processed when its `build` hook was
called. This _happened_ to work in some cases, but since Broccoli
builder uses a DAG to decide what order to build trees in it absolutely
could find a different sort order that causes `ModuleSourceFunnel` to
run before that styles tree.

As part of this, I updated broccoli-funnel to allow `super`ing with
either a single node (prior behavior) or an array. broccoli-funnel
itself will never look at ainputs other than `inputPath[0]` but this is
useful to ensure the broccoli node graph indicates the required
ordering.
@dfreeman
Copy link
Member

This change has landed in #257 and will be part of the forthcoming v2 release. Thank you @rwjblue!

@dfreeman dfreeman closed this Nov 22, 2021
@rwjblue rwjblue deleted the update-broccoli-funnel branch December 16, 2021 20:25
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.

2 participants