Skip to content
This repository has been archived by the owner on Mar 10, 2023. It is now read-only.

Update babel plugin name #6

Merged
merged 5 commits into from Aug 15, 2017

Conversation

adrianmcli
Copy link
Contributor

Fixes #5

Linting and tests all pass.

Copy link

@fgeorgsson fgeorgsson left a comment

Choose a reason for hiding this comment

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

Would it be possible for you to update the "dependencies" and "keywords" section to "babel-plugin-root-import" as well? It would remove the warning you get when you install this package due to the incorrect dependency.

@adrianmcli
Copy link
Contributor Author

@PCguru I was having trouble with switching over so I just left it. I can take a look at it again.

@adrianmcli
Copy link
Contributor Author

I've further illustrated my trouble with that in this issue on the babel-plugin-root-import repo: entwicklerstube/babel-plugin-root-import#66

@zurfyx
Copy link

zurfyx commented Feb 26, 2017

That's a breaking change!

IMO, you should either update package.json name as well and republish it, just like babel-root-import did. eslint-import-resolver-babel-plugin-root-import would make sense.

Or you should at least jump an entire version — to 1.0.0 — rather than a subversion like it is now. You don't want to break current app linters who are targeting ^0.0.2

@adrianmcli
Copy link
Contributor Author

@PCguru It seems that they suggest we keep using the old version. entwicklerstube/babel-plugin-root-import#66 (comment)

@zurfyx good point, I will do this.

@adrianmcli
Copy link
Contributor Author

I'll probably fork and republish if this isn't merged in sometime in the next few weeks.

@GollyJer
Copy link
Collaborator

GollyJer commented Apr 1, 2017

@adrianmcli is your branch working correctly? I'd love to get a working version of this bad boy if possible.
I tried simply updating the name in .eslitnrc but no luck.

@adrianmcli
Copy link
Contributor Author

@GollyJer yeah it should be working, I've been using it for the past few weeks. Give it a go and let me know.

@GollyJer
Copy link
Collaborator

GollyJer commented Apr 2, 2017

@adrianmcli Thanks for the help.
I can't get it to work. Clearly I don't know what I'm doing.

Here's what I tried.
"eslint-import-resolver-babel-root-import": "git://github.com/adrianmcli/eslint-import-resolver-babel-root-import.git#718368a629ef7c181a12a647224823faba5ac5d3"

.eslintrc

settings: {
    'import/resolver': {
      'babel-plugin-root-import': {
        rootPathPrefix: '~',
        rootPathSuffix: './_app',
      },
    },
  },

.babelrc

"plugins": [
    [
      "babel-plugin-root-import", // https://github.com/entwicklerstube/babel-plugin-root-import
      {
        "rootPathPrefix": "~",
        "rootPathSuffix": "./_app"
      }
    ]
  ],

@adrianmcli
Copy link
Contributor Author

@GollyJer you can see how I use it here: https://github.com/adrianmcli/next-boilerplate

@GollyJer
Copy link
Collaborator

GollyJer commented Apr 3, 2017

@adrianmcli Working perfectly, thanks!
It looks like others are taking a crack at it too.

@adrianmcli
Copy link
Contributor Author

@GollyJer haha, maybe it's time that I fork and publish an actual working option.

@GollyJer
Copy link
Collaborator

GollyJer commented Apr 3, 2017

@adrianmcli yeah, take a look at the version by @rmacklin. It seems like he's done a bunch of clean up too.

@rmacklin
Copy link

rmacklin commented Apr 3, 2017

Whoa, wasn't expecting anyone to see my fork, haha. I got that fork working on Thursday and was thinking about publishing it, but then I ended up switching approaches and dropping babel-plugin-root-import for a symlink inside node_modules because I liked the simplicity of that approach (which makes things like eslint and editor import detection "just work"). That said, I'd be happy to combine efforts on an updated version of this plugin.

A few things to note about your (@adrianmcli's) changes:

  • This line wouldn't handle if someone was using the plugin shorthand that allows you to omit the babel-plugin- prefix (i.e. if their plugins config had root-import rather than babel-plugin-root-import).
  • Also, this line would need to be updated to maintain support for babel config declared in a package.json.

But another thing I realized is that the getConfigFromBabel function doesn't handle if the plugin is only declared in the env section of your .babelrc. For that reason, one change I was trying on my fork was dropping getConfigFromBabel altogether. Instead, the options would be passed explicitly to the eslint plugin config (which can still be DRY if you're using .eslintrc.js and .babelrc.js with babel 7).

@michaeljonathanblack
Copy link

@rmacklin Could you provide a link to details on the symlink approach? I'm migrating a client app that used webpack path resolving to use server-side rendering and I need a server-workable path solution. Relative paths are just a nightmare with our feature-focused directory structure.

Thanks in advance!

Best
Michael

@michaeljonathanblack
Copy link

Ooh, some details around symlink, here: https://gist.github.com/branneman/8048520

A bummer that it's a solution that can't quite be committed, though.

@rmacklin
Copy link

@mherodev, a symlink can be committed actually, although I don't do that. Instead, I create the symlink in a postinstall script (and remove it in a preinstall script, so that it doesn't exist during the actual installation step, since that can cause issues).

@michaeljonathanblack
Copy link

michaeljonathanblack commented Jun 20, 2017

@rmacklin I don't hate the preinstall/postinstall script approach.

I took the solution available on the linked gist and updated it to use fs.stat over the deprecated fs.exists call:

    "preinstall": "node -e \"var d='node_modules/src',fs=require('fs');fs.stat(d,function(e){!e&&fs.unlinkSync(d)});\"",
    "postinstall": "node -e \"var s='../src',d='node_modules/src',fs=require('fs');fs.stat(d,function(e){e&&fs.symlinkSync(s,d,'dir')});\"",

This seems pretty straightforward and works. Thanks for the insight!

@rmacklin
Copy link

Seems like that'd work 👍 . In my project I just run a bash script rather than doing it in node, since I'm fortunate to not have to worry about cross-platform inconsistencies.

@camsjams
Copy link

Any plans on this being merged or published elsewhere under a new name?

@olalonde
Copy link
Owner

Anyone wants to be added to the repos' maintainers? I'm no longer using this plugin.

@GollyJer
Copy link
Collaborator

We're still using. I can take if you don't mind.

@olalonde
Copy link
Owner

@GollyJer added you as a collaborator on GitHub

@srghma
Copy link

srghma commented Aug 8, 2017

Can someone merge and publish it finally?

@adrianmcli
Copy link
Contributor Author

@BjornMelgaard I think @GollyJer is your best bet right now.

@srghma
Copy link

srghma commented Aug 8, 2017

@GollyJer, you are collaborator now, can you publish this fix))
P.S. @adrianmcli, thanks for boilerplate example, finally got it working)

@srghma srghma mentioned this pull request Aug 8, 2017
@GollyJer
Copy link
Collaborator

GollyJer commented Aug 9, 2017

I'm on vacation until the 12th. Will merge this asap.

@GollyJer GollyJer merged commit 86c7a03 into olalonde:master Aug 15, 2017
@GollyJer
Copy link
Collaborator

This is now merged and released.
I've never published to NPM. Is there a simple way to pass that torch or will you need to do it @olalonde?

@adrianmcli
Copy link
Contributor Author

@GollyJer glad to see this finally merged. You'll need access to publish the package on NPM. @olalonde will likely have to give you permission to do this. If he already did, then just bump the version number (while following SemVer) and do npm publish.

If you've never published anything on NPM before, I highly recommend you publish a test project just so you can see how the process works.

Thanks again for merging!

P.S. For those that want to use this immediately, you can replace the version number in your package.json with a link to this repo.

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.

None yet

9 participants