Relax the engines support in package.json for node6 support for fluent-syntax #164
Conversation
EnTeQuAk
changed the title
|
Hi @EnTeQuAk, thanks for the PR. The Node 8+ requirement was introduced in #95 and the motivation was #92 (comment). Node 6 doesn't support While I'll do my best to unblock mozilla/addons-linter#1789 in the short term. Perhaps we can relax the requirement for a few point releases? Long term -- what's your timeline for phasing out Node 6 support? Please say Firefox 61 :) |
|
BTW Is this a yarn-only thing? |
|
Ah, thanks for the hint regarding the compat transpilation. Well, it seems the compat build is working for the stuff that I use but it could break at other points it seems.
That depends on how Alternatively, I could fork the fluent-syntax package temporarily, rewrite whatever is node 8-only and publish that for the time being. I could even try using an older fluent-syntax package but I'd rather not go backwards here tbh. Or we could merge those changes temporarily but that increases maintainance burden on your end. What do you think? |
OK, this is great to know. I was mostly afraid of having different engine requirements across different packages in the repo. I'm sure down the road I'd forget and try to use some new JS feature which would break If we put a time limit on this, I think that's fine and easier to remember. I'll also try to set Travis up such that it runs |
This blocks to mozilla/addons-linter#1789 (and thus the langpack signing for Firefox 60). fluent-syntax is compatible with node 6 when using the compat build but still can't be installed given that it requires node 8+ in the packages.json. Can we relax that a little bit? Addons-linter is a dependency of `web-ext` which is being installed on end-users machines and thus has a bit more LTS requirements and has to support node6 while it's still LTS.
By default, make test requires babel-polyfill and all tests run fine even on older versions of Node. For fluent-syntax, howeever, we want to also run tests without the polyfill in order to make sure that fluent-syntax is actually runnable on Node 6. See projectfluent#164.
By default, make test requires babel-polyfill and all tests run fine even on older versions of Node. For fluent-syntax, howeever, we want to also run tests without the polyfill in order to make sure that fluent-syntax is actually runnable on Node 6. See projectfluent#164.
stasm
force-pushed
the
support-node6
branch
2 times, most recently
from
f8db290
to
a96e6e6
Compare
|
@Pike Does this look like a sane way forward until May? |
| @@ -23,6 +23,10 @@ compat.js: $(SOURCES) | |||
| --output.file $@ | |||
| @echo -e " $(OK) $@ built" | |||
|
|
|||
| .PHONY: test-without-babel-polyfill | |||
| test-without-babel-polyfill: | |||
| @mocha --recursive --ui tdd --require babel-register test/ | |||
There was a problem hiding this comment.
We still need babel-register to transpile ES6 syntax, just like the compat builds do. Thanks to this, imports work correctly, for instance.
.travis.yml
Outdated
| script: make deps && make dist | ||
| script: | ||
| - make deps && make dist | ||
| - if [[ $(node --version) = v6.* ]]; then make -C fluent-syntax test-without-babel-polyfill; fi |
There was a problem hiding this comment.
Any reason to only test this for v6 and not for v8 as well?
There was a problem hiding this comment.
No reason, except that I fixated on Node 6 :)
By default, make test requires babel-polyfill and all tests run fine even on older versions of Node. For fluent-syntax, howeever, we want to also run tests without the polyfill in order to make sure that fluent-syntax is actually runnable on Node 6.
This blocks mozilla/addons-linter#1789 (and thus the langpack signing
for Firefox 60).
fluent-syntax is compatible with node 6 when using the compat build but
still can't be installed given that it requires node 8+ in the
packages.json.
Can we relax that a little bit?
Addons-linter is a dependency of
web-extwhich is being installed onend-users machines and thus has a bit more LTS requirements and has to
support node6 while it's still LTS.
I added the
ignore-enginesoption to the addons-linter tests to check that the build really does support node 6 and all tests are passing.See #164 for more details.