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

Add .babelrc to .npmignore #16

Merged
merged 1 commit into from Feb 8, 2018
Merged

Conversation

jskorepa
Copy link
Contributor

@okonet
Copy link
Collaborator

okonet commented Jan 19, 2018

You might want to skip code from node_modules in Jest config (this is default BTW) instead of trying to fix all npm modules :)

@ajsharp
Copy link
Contributor

ajsharp commented Feb 7, 2018

After reading @jskorepa's issues on react-dropzone this sounds like it would fix the issue I'm having as well. Where do you stand on this @okonet? I'm not familiar with what's considered best practice in the community for publishing build configuration in the distributed package.

I agree this is probably the result of an overly optimistic assumption in parcel, and should maybe be changed there. Still, unless there's a good reason to ship the .babelrc file in the npm package, would you consider leaving it out?

@okonet
Copy link
Collaborator

okonet commented Feb 7, 2018

I never used parcel before but it looks like something they should fix. I’m okay with merging this, though.

@okonet
Copy link
Collaborator

okonet commented Feb 7, 2018

Please sync the branch and I’ll merge it

@jskorepa
Copy link
Contributor Author

jskorepa commented Feb 7, 2018

Rebased to current master. For anyone looking for solution which does not require patching lot of npm packages I ended up setting following script as "prepare" in package.json

const fs = require('fs')
const path = require('path')

for (const dep of fs.readdirSync('node_modules')) {
  const dir = path.join('node_modules', dep)
  const file = name => path.join(dir, name)
  // remove .babelrc
  try {
    const from = file('.babelrc')
    const to = file('.babelrc.bckp')
    fs.renameSync(from, to)
    console.log('Renamed', from, 'to', to)
  } catch (e) {
    // ignore error
  }
  // patch package.json to remove babel spec
  try {
    const pkg = JSON.parse(fs.readFileSync(file('package.json')))
    if (pkg.babel) {
      delete pkg.babel
      fs.renameSync(file('package.json'), file('package.json.bckp'))
      fs.writeFileSync(file('package.json'), JSON.stringify(pkg, 0, 2), 'utf-8')
    }
  } catch (e) {
    // ignore error
  }
}

@okonet okonet merged commit 28aa14c into react-dropzone:master Feb 8, 2018
@okonet
Copy link
Collaborator

okonet commented Feb 8, 2018

Sorry for bike shedding, but why are you using parcell? Wasn't it zero-congif in the first place? So now you need to patch the whole internet to get a tool working? 🤔

@sapegin
Copy link

sapegin commented Feb 8, 2018

This is actually a very good thing to do but for another reason: you shouldn't publish unnecessary files (any tooling configs for example) to npm.

And webpack has the same issue, though it's easy to fix.

@okonet
Copy link
Collaborator

okonet commented Feb 8, 2018

@sapegin I'm not sure about this in case you want to build from source (which is the case with webpack, afaik).

@ajsharp
Copy link
Contributor

ajsharp commented Feb 8, 2018

For anyone interested, the main discussion thread for this issue in parcel is here: parcel-bundler/parcel#13.

I agree that parcel should not be doing this. Thanks @okonet for accommodating this change regardless!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants