Add 'node: current' to the targets file on install

[mansona]

This is something that we discussed at the last fastboot weekly meeting, essentially it is designed to fix the issue where people have errors with nullish coalescing during a fastboot build because their targets file only includes browsers that natively support the feature and it is not getting transpiled.

This PR adds the node: "current" to the targets file, which will always refer to the current node version that is running the build. You can read more here: https://babeljs.io/docs/en/babel-preset-env#targetsnode

I thought I would submit the working implementation before we discussed how to test this 🤔 maybe a topic for this week's meeting.

There are some issues with this implementation, even though I know it works (and I've been using something similar for quite some time). The first issue that might be a blocker but I don't know, is that it doesn't prompt you like normal blueprint changes to see what the diff is and ask if you want to apply it 🤔 because it's writing outside of the context of ember-cli it just writes the changes to the file directly. I don't actually know how to fix this

[rwjblue]

I'm generally 👍 on the direction here, left a few small inline comments.

[rwjblue]

In order to get the diffing, we'd need to do this before afterInstall. There are a few ways, but they all kinda suck.

I think the simplest / best thing would be to dynamically generate the file in a temp directory before calling super in install and using that generated directory as our filesPath.

[rwjblue]

Shared a small snippet in discord to try to explain better what I meant:

filesPath() {
  return this._filesPath;
},

install() {
  // make a temp dir, probably with tmp
  // do your recasty stuff into that directory
  this._filesPath = tmp.dirSync();
  
  this._super.install.apply(this, arguments;
}

[rwjblue]

This path is not guaranteed to be the correct location for targets. We should follow the logic in ember-cli for this:

https://github.com/ember-cli/ember-cli/blob/614c854171d64375ef904861119e9e2d69f75d9f/lib/models/project.js#L275-L281

[rwjblue]

Similarly, I think that this is configurable.

[rwjblue]

Changes look good, thank you! I'd like to also add a blueprint test or two for this before landing. I think we normally would use https://github.com/ember-cli/ember-cli-blueprint-test-helpers for those kinds of tests...

[mansona]

so... I took a quick look at the tests you mentioned @rwjblue and at first it looked promising... but then I hit an issue where we're getting a prompt during the test 🙈 It seems like it's a known issue and not currently possible ember-cli/ember-cli-blueprint-test-helpers#65

Any ideas?

[rwjblue]

Hmm, what prompt are we getting?

[mansona]

the one that I just coded into it 😂 the "do you want to change targets.js" prompt:

[mansona]

I pushed a WIP of the test so that we can see the prompt I'm talking about: https://github.com/ember-fastboot/ember-cli-fastboot/pull/770/checks?check_run_id=910533204#step:6:15

[mansona]

yikes 😬 it looks like it was running for 5 hours before it was cancelled https://github.com/ember-fastboot/ember-cli-fastboot/pull/770/checks?check_run_id=910533204

Is there any way to shorten that so it doesn't use up all our free minutes?

[rwjblue]

Is there any way to shorten that so it doesn't use up all our free minutes?

ya, we can set a timeout

[mansona]

ya, we can set a timeout

So... there is already a timeout defined in mocha of 20000ms but it doesn't seem to actually cancel the emberGenerateDestroy() call if the mocha timeout is hit 🤔 I'm assuming that this is due to the way that the prompt is implemented in ember-cli or something

If this is something that you think we need to fix upstream just let me know 😂 I'm always happy to do a bit of yak shaving to get something done and make the whole pipeline a bit better along the way 💪

[mansona]

@rwjblue just a bit of a status report on this test (other than the fact that it will probably be the end of me 😫 )

I have tried what you suggested in our last meeting with listening into stdout and trying to write to stdin... but they don't seem to be working at all 😞 I am getting the feeling that they are using different streams in this setup (maybe because of mocha or something who knows 🤷 ) but I have confirmed that writing y to the stdin at the right time doesn't seem to do anything in our scenario.

If we still need this to be tested then I think the only option is to expand on ember-cli-blueprint-test-helpers to allow it to confirm prompts or figure out some way to mock the console-ui in the same way that ember-cli does.

I'm going to spend a bit more time investigating but any advice would be appreciated 👍

[lionelrudaz]

Yes, thanks everyone, it's working well with node: "current" added to my targets.

I have to say that I feel only half dumb on this one. I mean, I'm working with Ember on several side projects for a long time now, but it's the first time I encounter such an issue. As soon as you turn Fastboot on, you've got this feeling that your app will break any time... Any way I can help?

[simonihmig]

Hey everybody, just chiming in to mention that I believe I already had a handful of issues raised / questions asked in discord regarding the use of nullish coalescing in ember-bootstrap, blaming the addon for breaking FastBoot. And apparently - looking at the added references here - other addon authors face this as well. So would be really cool to get this landed! 😍

I have not followed this deeply, so is it "just" that this feature is currently not testable what is holding this up? If so - just my 2 cents here - I would rather get this in with no or just manual testing, than to substantially delay this further.

Oh, and thanks @mansona for working on this! 🙏

[mansona]

We have discussed this issue in the Fastboot meeting and we've decided to get this landed and not block it on getting the specifics of the tests working 🎉

I have opened up a new PR to track the testing work here #789

[rwjblue]

Just a note: we've moved the tests here to another PR #789 so that we can get this fix out while we figure out how to make the tests work properly.