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

Bring back Node 8 support (for now) #1116

Merged
merged 2 commits into from
Dec 20, 2019
Merged

Bring back Node 8 support (for now) #1116

merged 2 commits into from
Dec 20, 2019

Conversation

chancancode
Copy link
Member

Revert some dependencies bump from #1096 because ember-electron (probably the only npm consumer?) still support Node 8. This is mostly a technicality, since they don't actually run the build and only consume the artifacts in dist (included in the npm package).

We should probably find a better way for them to consume the app, but in the mean time, this will allow them to consume the latest inspector until Node 8 is officially EOL in a few weeks. We can try this again when Ember CLI drops Node 8 support (likely 3.17).

@chancancode chancancode requested a review from rwjblue December 20, 2019 18:08
@chancancode
Copy link
Member Author

cc @bendemboski

@chancancode
Copy link
Member Author

chancancode commented Dec 20, 2019

@bendemboski just to set the expectation – in my personal opinion (other maintainers may disagree?) I doubt we will do a major version bump just to drop Node 8 support in the inspector, since that would signal the "wrong thing" to most consumers who consume this as an auto-updating browser extension. Not that the version number means anything there, but it does show up pretty prominently in the "What's New" screen. I think we can do at least minor bump (like this one) though. Just bringing this up since you may want to pin to ~ instead, for your purpose.

We should probably find you a better way to consume the package. The NPM build is not very coherent right now in my opinion, and this is one way that it surfaces. The inspector is really more of an "app" than a "library" in the traditional sense, and we have some struggles with trying to express both things in a single package(.json). We may be better off switching to something like an S3 build that you can pull in.

Revert some dependencies bump from #1096 because `ember-electron`
(probably the only npm consumer?) still support Node 8. This is
mostly a technicality, since they don't actually run the build and
only consume the artifacts in `dist` (included in the npm package).

We should probably find a better way for them to consume the app,
but in the mean time, this will allow them to consume the latest
inspector until Node 8 is officially EOL in a few weeks. We can
try this again when Ember CLI drops Node 8 support (likely 3.17).
@bendemboski
Copy link
Contributor

@chancancode yeah, completely agreed. Since this project isn't really intended to be consumed as a node module or run on Node by end users, it doesn't make sense to major version bump just because of a change in (what should be only) build-time Node support.

I agree with you that really we should find a better way for ember-electron to consume ember-inspector and I'm happy to make some time in the near future to do whatever is needed on the ember-electron side.

FWIW, here is the code that installs ember-inspector. So the Node support thing is kinda silly because I'm pretty sure none of that code relies on the Node version at all -- the only problem is the install-time engines checks. I already want to rework how it's installed a bit, and was already planning to stop making ember-inspector a dependency of ember-electron, and instead install it in the Electron project that ember-electron generates when running its blueprint. That would allow me to install ember-inspector on its own with an --ignore-engines flag that would at least get us over this hurdle.

But I know you'd like to stop publishing all the build artifacts to npm, so it's probably worth discussing a more full solution so we stop using the npm registry at all. Perhaps we should discuss on Discord, or maybe in a separate issue either in ember-inspector or ember-electron?

And again, thank you very much for making this short-term solution happen!

@chancancode
Copy link
Member Author

Yes we should discuss more! 😄 I think your diagnosis is exactly right, I think none of this actually matters but it just unfortunately cogs up the engine (no pun intended) here, and since this is an important update and there are otherwise no reasons for us to drop Node 8 support, I think it's totally justified to revert this so electron users can benefit from the update.

As far as publishing to npm or not – as far as I am aware, ember-electron is the only consumer so it's really just whatever is the most convenient for both of us. I think publishing only the build artifacts to npm actually makes some sense, it's like how jQuery, moment.js or other browser-based libraries uses NPM just as a delivery mechanism. It may also make some sense to publish the unbuilt app if there is a way to consume that.

What doesn't make sense is doing both at the same time in the same package, especially that is definitely no way to consume the app as an npm package today without doing a lot of hacks 😄

@bendemboski
Copy link
Contributor

I think publishing only the build artifacts to npm actually makes some sense

Very much agreed! That allows me to not worry about building some other mechanism to download the artifacts and get updates and whatnot, and also allows me to enable users to manage the ember-inspector version/updates their self using the familiar npm/yarn semantics.

@chancancode chancancode merged commit 09552b0 into master Dec 20, 2019
@delete-merged-branch delete-merged-branch bot deleted the node-8-support branch December 20, 2019 18:52
chancancode added a commit that referenced this pull request Dec 20, 2019
Revert some dependencies bump from #1096 because `ember-electron`
(probably the only npm consumer?) still support Node 8. This is
mostly a technicality, since they don't actually run the build and
only consume the artifacts in `dist` (included in the npm package).

We should probably find a better way for them to consume the app,
but in the mean time, this will allow them to consume the latest
inspector until Node 8 is officially EOL in a few weeks. We can
try this again when Ember CLI drops Node 8 support (likely 3.17).

(cherry picked from commit 09552b0)
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 1, 2020
Revert some dependencies bump from emberjs#1096 because `ember-electron`
(probably the only npm consumer?) still support Node 8. This is
mostly a technicality, since they don't actually run the build and
only consume the artifacts in `dist` (included in the npm package).

We should probably find a better way for them to consume the app,
but in the mean time, this will allow them to consume the latest
inspector until Node 8 is officially EOL in a few weeks. We can
try this again when Ember CLI drops Node 8 support (likely 3.17).
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 5, 2020
Revert some dependencies bump from emberjs#1096 because `ember-electron`
(probably the only npm consumer?) still support Node 8. This is
mostly a technicality, since they don't actually run the build and
only consume the artifacts in `dist` (included in the npm package).

We should probably find a better way for them to consume the app,
but in the mean time, this will allow them to consume the latest
inspector until Node 8 is officially EOL in a few weeks. We can
try this again when Ember CLI drops Node 8 support (likely 3.17).
patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
Revert some dependencies bump from emberjs#1096 because `ember-electron`
(probably the only npm consumer?) still support Node 8. This is
mostly a technicality, since they don't actually run the build and
only consume the artifacts in `dist` (included in the npm package).

We should probably find a better way for them to consume the app,
but in the mean time, this will allow them to consume the latest
inspector until Node 8 is officially EOL in a few weeks. We can
try this again when Ember CLI drops Node 8 support (likely 3.17).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants