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

processImports set to false and using @import results in TypeError: Cannot read property 'rules' of undefined #3504

Open
vandernorth opened this issue May 26, 2020 · 8 comments

Comments

@vandernorth
Copy link

I've looked through the issues, but couldn't find something similar. The below code used to work in less 2.x.

const less = require('less');
const raw = ` @import url('https://fonts.googleapis.com/css?family=Open+Sans:400,700');`;

less.render(raw, { processImports: false })
    .then(() => console.info('Less ok!'))
    .catch(error => console.error('Less failed!', error));

When trying this with less 3.11.1 it fails with TypeError: Cannot read property 'rules' of undefined on the following less-code from less.cjs.js:

else {
  ruleset = new Ruleset(null, copyArray(this.root.rules));
  ruleset.evalImports(context);
  return this.features ? new Media(ruleset.rules, this.features.value) : ruleset.rules;
} 

this.root seems to be undefined. I'm just wondering if it's something I did wrong on my end or that it's a bug we can fix?

Setting processImports to true > Ok
Adding more css/less > Same error.

Managed to fix it by changing the less code to

else if (this.root) {
  ruleset = new Ruleset(null, copyArray(this.root.rules));
  ruleset.evalImports(context);
  return this.features ? new Media(ruleset.rules, this.features.value) : ruleset.rules;
} else { return []; }

But I'm not sure if that's the best approach here, there might be an underlying bug somewhere.

@matthew-dean
Copy link
Member

Can you do a PR with a test case that fails in 3.11.1 and succeeds with your PR?

@vandernorth
Copy link
Author

@matthew-dean I've checked and the error no longer occurs, but now the whole imports are gone. In v2 the import would be reflected in the output (instead of processing it). Do you want me to create a new issue for that?

@matthew-dean
Copy link
Member

@vandernorth Damn it, I must not have written the appropriate test. No, let's re-open this one. [sigh]

@matthew-dean matthew-dean reopened this Jul 30, 2020
@matthew-dean
Copy link
Member

matthew-dean commented Jul 30, 2020

@vandernorth Hang on, are you speaking of this, which is fixed? #3508

OR are you saying it's empty with processImports: false on? So, was only partially fixed?

@matthew-dean
Copy link
Member

matthew-dean commented Jul 30, 2020

@vandernorth

I'm trying to figure out what the intended behavior of processImports is. It doesn't seem to be documented here: http://lesscss.org/usage/, and in the code, it is sort of vague as to the output implications.

So, can you point to where you found this option documented, how you are you using it, and how you expect this to function in regards to output?

@vandernorth
Copy link
Author

vandernorth commented Aug 5, 2020

@matthew-dean I'm not sure where I've found this options. Must have been from an example or StackOverflow answer like this one.

The reason we need/use this option is because we allow users to create their own LESS and we parse it for them, server-side. The normal behavior will allow things like @import('/etc/passwd') and it will include the file as text.

Setting processImports to false would (in v2) leave the imports as-is. So don't process them, just copy them over to the output, and let the browser handle the import.

I hope this answers your question, if you have any additional questions, let me know!

@matthew-dean
Copy link
Member

@vandernorth In that case, I think it needs a PR with proper tests?

@matthew-dean
Copy link
Member

@vandernorth So, in diving into the codebase for the last month, it appears like processImports is simply an option set by the import visitor to not create duplicate visitors, and that it's not actually a supported option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants