travis build error due to nunjucks

[curbengh]

this is to discuss various potential workarounds for travis build error in #795 #796 #797 and newer PRs. This is due to incorrect syntax used by nunjucks and has been reported upstream mozilla/nunjucks#1160.

The workaround will be temporary and can be reverted once the fix mozilla/nunjucks#1161 is released.

@tomap suggests to add --ignore-engines flag to yarn install.
@yoshinorin suggests specifying "lts" node version in travis.

the latest lts is going to be 10.x, starting from 2018-10-30 (nodejs/Release).

I personally suggest specifying "10" as "lts" could be confused with node 8.

[yoshinorin]

Thanks @weyusi .

I personally suggest specifying "10" as "lts" could be confused with node 8.

That's fine too.
One thing, I can say we shoud stop to use - "node" option at travis ci.

[curbengh]

you mean stop using - "node" temporarily (to fix this) or permanently?

[tomap]

As long as there is an issue with node 11. Then we'll see. Let's not make plans for 2030 :) Le mar. 30 oct. 2018 à 01:51, weyusi <notifications@github.com> a écrit :

…

you mean stop using - "node" temporarily (to fix this) or permanently? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#798 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJY_lLW5CNv_c8LvXf8ZlPud3RZjskhks5up6KDgaJpZM4X-9JZ> .

[curbengh]

this issue aside, I thought having "node" is to test the website against latest version of node? I assume the actual (netlify) deployment uses lts (netlify says it's 8, but could be 10) instead.

since travis is only for testing, shouldn't 6 and 8 be tested as well? I wanna clarify this if we're going to update travis config.

[yoshinorin]

you mean stop using - "node" temporarily (to fix this) or permanently?

Permanently. Because, I think same problem will occur when next Node.js release.

this issue aside, I thought having "node" is to test the website against latest version of node?

Maybe no. (I don't know why we set "-node")

since travis is only for testing, shouldn't 6 and 8 be tested as well?

I agree your opinion.

My english is not good. It's sorry if my answer is not has a point 😓

[tomap]

Regarding the last part:
"
since travis is only for testing, shouldn't 6 and 8 be tested as well?
"
We are testing to make sure it can be deployed, so if we are deploying with node 10, no need to test earlier versions (6 or 8)

Now that we agree, do you want to use node 10 or lts (it is currently the same)?

[curbengh]

@yoshinorin @tomap thanks for clarifying! I think the travis should use similar node version as netlify, there are two ways:

1. Config netlify to use similar version as travis. e.g. set lts in .nvmrc file (for netlify) and travis.
2. Ask netlify (I'm emailing) whether they use 8, 10, or lts by default; set the value in travis.

For consistency, I prefer the first approach and using lts.

[curbengh]

netlify is using 8
https://github.com/netlify/build-image/blob/42bca793ccd33055023c56c4ca8510463a56d317/Dockerfile#L266-L273

Shall we use lts in travis and netlify?

[yoshinorin]

Shall we use lts in travis and netlify ?

You means lts is v10 or v8 ? which ? 🤔
I understanding currently lts means v10 (After Oct 31)

[NoahDragon]

@yoshinorin @weyusi I think it is due to the nunjucks@3.1.3 not support the latest node. The hotfix has committed ef771de

Long term solution would be to wait the nunjucks to update, then change back the node version to be latest.

[curbengh]

@yoshinorin lts should be 10 by now. both travis and netlify use nvm, so even if lts is still 8, both of them would use similar node version.

@NoahDragon is netlify configured to use latest node? shouldn't travis and netlify use similar node version?

[SukkaW]

@weyusi @NoahDragon
Netlify uses Node 8 as its default engine, while you can defined which version to use in .nvmrc.

Travis CI supports .nvmrc as well, see related travis-ci's docs for more details.

[curbengh]

@SukkaW Thanks for the clarification.
I think it would be more convenient to use lts for both, instead of switching the node version back and forth (in travis) every time this issue comes up. It should give plenty of time for deps to update their engines: <= x.x.x.

Edit: I now prefer specifying version (e.g. 10) because lts may mean more than one versions.