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

Null reference issue run/watch on scss change #1160

Closed
wants to merge 1 commit into from

Conversation

michaelwjackson
Copy link

@michaelwjackson michaelwjackson commented Jan 30, 2020

I have a scenario where bundled scss->css files that are not directly referenced by the aurelia app but are loaded dynamically based on user-selected theme with references to other scss files. When I do an "au run" and then modify one of these files, which are referenced in the aurelia project json's cssProcess source array glob to trigger rebuild, it does pick up the change, but then the code crashes on rebuild due to a null reference. I dug in and the issue turns out to be this.includedIn in the bundled-source.js file update(file) method. Doing a simple null check fixes everything for my scenario.

I have a scenario where a bundled scss->css files that are not directly referenced by the aurelia app but are loaded dynamically based on user-selected them have references to other scss files.  When I do an au run and then edit one of these files, which are referenced in an aurelia project cssProcess source array glob to trigger rebuild does, but then the code crashes on rebuild due to a null reference, which turns out to be this.includedIn in the bundled-source.js file.  Doing a simple null check fixes everything for my scenario.
@3cp
Copy link
Member

3cp commented Jan 30, 2020

Can you clarify where is your dynamically loaded scss files? My guess is that they are not in src/ folder.

The includedIn points to the bundle itself. It should exist, unless your scss/css files are not in the final bundle files (but rather saved as individual css files through gulp.dest).

Can you share your aurelia.json?

@MichaelPetrinolis
Copy link
Contributor

@3cp I suppose you mean @michaelwjackson :)

@3cp
Copy link
Member

3cp commented Jan 30, 2020

Sorry about that :-)

@michaelwjackson
Copy link
Author

michaelwjackson commented Feb 3, 2020

@3cp,

These files are indeed outside the src folder, though the final css files are inside. I moved them into the folder at one point and tried it, but got what I thought was the same error. Perhaps it was something else. I would definitely be able to put them inside, though in this case it isn't preferable. There have been times I've made references to files under node_modules, so it seems like a case that isn't uncommon, though admittedly you wouldn't really edit those and expect a watch to refresh them. I've got a pre-build step that modifies the aurelia cli code file in my node_modules on the fly, so I have a workaround in my project. I just figured a null check wasn't the worst thing to have here.

Also, I've modified my src file to be aurelia to be less confusing, as I have src at another level in the hierarchy. There is also a decent chunk of non-aurelia stuff in this same Visual Studio project, wherein lies this outside reference.

aurelia.json.txt

@michaelwjackson
Copy link
Author

michaelwjackson commented Feb 3, 2020

@3cp, in thinking about it, I've also possibly modified my css gulp task to more aggressively rebuild when any scss file is edited. I've been using slightly modified gulp tasks for several projects and would have to compare them back to the originals to see if this is one of them. The build works fine for me, it's just the watch on run.

tasks.zip

@3cp
Copy link
Member

3cp commented Feb 3, 2020

I am not against the null check. Just want a full understanding what's going on. I will go through your setup tomorrow.

@3cp
Copy link
Member

3cp commented Feb 4, 2020

What's the scss file path caused the null includedIn?

Currently there is a logic hole in cli-bundler. When a file is not matched by any bundle "source": [ ] list, it's left out from all bundle files. I suspect all your bundles "source" matchers missed out some theme files.

The missed scss file will not land in any bundle files. Which tells me possibly:

  1. you didn't use or rely on that missed file at runtime.
  2. you have other method to copy that missed (processed css) file to a dist folder which you manually fetch it at runtime.

The null check can bypass the error, but it would not magically bring back the file to a bundle. That tells me you didn't need that file in bundle anyway.

Logically, cli-bundle should capture any missed file into the last vendor-bundle (aka target bundle) file. But the current architecture is not easy to add that support.

@3cp
Copy link
Member

3cp commented Feb 19, 2020

@michaelwjackson can you provide bit more information on how you use that missing (if it did miss bundle) css file at runtime?

@@ -91,7 +91,7 @@ exports.BundledSource = class {
this.file = file;
this._contents = null;
this.requiresTransform = true;
this.includedIn.requiresBuild = true;
if (this.includedIn) this.includedIn.requiresBuild = true;
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest printing a warning, if this makes sense.

if (this.includedIn) {
  this.includedIn.requiresBuild = true;
} else {
  logger.warn(this.path + ' is not captured by any bundle file. You might need to adjust the bundles source matcher in aurelia.json.');
}

@michaelwjackson
Copy link
Author

@3cp Sorry I've been pulled off of this particular pet project for the past few weeks for two paying gigs that have been consuming my time. I will try to get a copy of the relevant pieces of source code stripped down to the very minimal pieces as an example to help soon. This will also be edifying to me to ensure it's not something else I'm doing or something silly I'm missing.

@3cp
Copy link
Member

3cp commented Mar 12, 2020

Thanks for working on this issue. Take your time. This is not an urgent bug to be fixed.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@3cp
Copy link
Member

3cp commented May 14, 2020

Replaced by #1183

@3cp 3cp closed this May 14, 2020
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.

4 participants