Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Dec 14, 2020

Requirements for Contributing a Bug Fix

Identify the Bug

As of the second-to-last commit in #31 (6e6ad69), babel isn't meant to run in this repository anymore. babel isn't available in the devDependencies. However, the rest of the repository expects babel to "compile" (mostly copy-paste) the files from src/ to lib/, which isn't happening anymore as of #31.

The current state of the repository causes the "build" and "prepare" scripts to error out, as they had been running babel to compile the source code, but it is missing from the devDependencies.

This means npm install (and npm publish) are broken in this repository, and CI is failing.

Description of the Change

As mentioned already in #31, the code in this repository doesn't actually need to be compiled anymore in order to be compatible with modern Node.js.

This Pull Request simply moves the JS files fromsrc/ to lib/ where this package and its consumers expect them to be... and cleans up the repository in various ways:

  • Delete the build and prepare scripts in package.json, which previously ran babel on the src/ files. These scripts aren't needed anymore.
  • Adjust the test files to expect the files to be in lib/, not src/.
  • Stop .gitignoreing the lib directory, as that's where all the code lives now in the repository.
  • Stop .npmignoreing the src directory, as it doesn't exist anymore.
    • Off-topic for this PR, but started .npmignoreing appveyor.yml, because consumers of this package don't need our CI config file.

Alternate Designs

None.

Possible Drawbacks

This PR means babel isn't transforming the code anymore.

If babel was meaningfully changing the functionality of the code (which it shouldn't), then the code will behave slightly different now.

Verification Process

CI passes... it would be nice to see if Atom builds and passes its tests with this change as well.

Release Notes

N/A

We don't need to compile anything anymore, apparently,
so just move the "source" files to lib/.
There shouldn't be any scripts for compiling the code
anymore. This makes `npm install` and `npm publish` work again.
This is the main code directory of the repo now.
It should not be .gitignored anymore.
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Dec 14, 2020

pinging @aminya and @sadick254...

This PR finishes something that was started in #31 (6e6ad69) and makes CI passing again for this repository.

Let me know what you think, and feel free to merge this if it looks good.

"tdd": "electron-mocha test/**/*.test.js --ui=tdd --renderer --interactive",
"build": "rimraf lib && babel src -d lib",
"prepare": "npm run build"
"tdd": "electron-mocha test/**/*.test.js --ui=tdd --renderer --interactive"
Copy link
Contributor

@aminya aminya Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove build and prepare.

"name": "electron-link",
"version": "0.5.0",
"description": "Generates scripts to pass to V8's `mksnapshot` tool.",
"main": "lib/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"main": "lib/index.js",
"main": "src/index.js",

Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch.

My suggestions are what I should have done. I don't understand why the files are moved.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Dec 14, 2020

That would work.

I'm not sure if anyone uses this package and manually uses require(electron-link/lib/[some-file]), but it would be a breaking change for those users.

I don't care strongly either way, but maybe this is a major version bump kind of change for semver purposes.

For the record, this package is used outside of Atom in various other projects: https://github.com/atom/electron-link/network/dependents (about three or four non-Atom projects if you page through)

@DeeDeeG DeeDeeG marked this pull request as draft December 14, 2020 04:00
Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. It is better if we transfer src to lib and delete src. As @DeeDeeG has pointed out this package is used outside of Atom in various other projects.

@sadick254 sadick254 marked this pull request as ready for review December 14, 2020 19:37
@sadick254 sadick254 merged commit e093e5f into atom:master Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants