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

Bump magic-string #184

Closed
wants to merge 4 commits into from
Closed

Bump magic-string #184

wants to merge 4 commits into from

Conversation

curran
Copy link
Contributor

@curran curran commented Feb 13, 2019

Closes #183

@curran
Copy link
Contributor Author

curran commented Feb 13, 2019

Oh no! CI is failing, but only for Node 4 and 6.

These are the failing tests:

    modules.js
      ✓ disallows import statement
      1) disallows export statement
      ✓ imports are ignored with `transforms.moduleImport === false`
      2) exports are ignored with `transforms.moduleExport === false`
      3) imports and exports are ignored with `transforms.modules === false`
      ✓ Supports anonymous functions as default export
      ✓ Supports anonymous classes as default export

  1) buble
       modules.js
         disallows export statement:
     SyntaxError: Export 'foo' is not defined (1:9)
  1 : export { foo };
               ^
      at null.pp$4.raise (node_modules/acorn/src/location.js:15:13)
      at null.pp$1.parseTopLevel (node_modules/acorn/src/statement.js:27:12)
      at null.parse (node_modules/acorn/src/state.js:92:17)
      at Function.parse (node_modules/acorn/src/state.js:112:37)
      at Object.transform (src/index.js:56:16)
      at /home/travis/build/Rich-Harris/buble/test/test.js:53:16
      at _tryBlock (assert.js:320:5)
      at _throws (assert.js:339:12)
      at Function.assert.throws (assert.js:369:3)
      at Context.<anonymous> (test/test.js:52:16)
  2) buble
       modules.js
         exports are ignored with `transforms.moduleExport === false`:
     SyntaxError: Export 'foo' is not defined (1:9)
  1 : export { foo };
               ^
      at null.pp$4.raise (node_modules/acorn/src/location.js:15:13)
      at null.pp$1.parseTopLevel (node_modules/acorn/src/statement.js:27:12)
      at null.parse (node_modules/acorn/src/state.js:92:17)
      at Function.parse (node_modules/acorn/src/state.js:112:37)
      at Object.transform (src/index.js:56:16)
      at Context.<anonymous> (test/test.js:57:16)
  3) buble
       modules.js
         imports and exports are ignored with `transforms.modules === false`:
     SyntaxError: Export 'foo' is not defined (1:23)
  1 : import 'foo'; export { foo };
                             ^
      at null.pp$4.raise (node_modules/acorn/src/location.js:15:13)
      at null.pp$1.parseTopLevel (node_modules/acorn/src/statement.js:27:12)
      at null.parse (node_modules/acorn/src/state.js:92:17)
      at Function.parse (node_modules/acorn/src/state.js:112:37)
      at Object.transform (src/index.js:56:16)
      at Context.<anonymous> (test/test.js:57:16)

@Rich-Harris Any insights here as to why magic-string version upgrade would induce these failing tests?

@adrianheine
Copy link
Member

That's actually a change in acorn.

@curran
Copy link
Contributor Author

curran commented Feb 15, 2019

Aha! Excellent insight! Thank you @adrianheine .

@adrianheine
Copy link
Member

I just pushed 77ab236 to fix that.

@curran
Copy link
Contributor Author

curran commented Feb 15, 2019

Closing in favor of #185

@curran curran closed this Feb 15, 2019
@adrianheine
Copy link
Member

I think this one could be merged much quicker than #185, see my comments there.

@curran curran reopened this Feb 15, 2019
@curran
Copy link
Contributor Author

curran commented Feb 15, 2019

@adrianheine Makes sense. I merged latest master, should be good to go.

@adrianheine
Copy link
Member

Most of your changes to package-lock.json come from us using different npm or node versions, I guess. If I pull your branch and run npm i they are all reverted.

@curran
Copy link
Contributor Author

curran commented Feb 15, 2019

Yes, I was a bit confused about those changes myself. I apologize if they cause problems.

These are the Node and NPM versions I'm on:

$ node --version
v11.9.0
$ npm --version
6.5.0

Perhaps best to use your generated version of package-lock, with fewer changes?

@adrianheine
Copy link
Member

$ node --version
v10.15.1
$ npm --version
5.8.0

Yeah, keeping my version would be nice since I'm currently the person doing most of the commits in this project. Ideally a package-lock.json would not be so volatile, but well …

@curran curran mentioned this pull request Feb 15, 2019
@curran
Copy link
Contributor Author

curran commented Feb 15, 2019

Closing in favor of #186 , as the commits here would make a mess in Git history.

@curran curran closed this Feb 15, 2019
@adrianheine
Copy link
Member

btw, I'm absolutely willing to invest some work squashing otherwise good commits, and you could have used git add -p for picking only the parts of your changes we want to merge.

@curran
Copy link
Contributor Author

curran commented Feb 16, 2019

Ah, good to know for future work. Thanks for the tips!

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

2 participants