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 charset so tests work with convert-source-map@>1.4 #5302

Merged
merged 1 commit into from Feb 13, 2017

Conversation

loganfsmyth
Copy link
Member

Fix a test broken by the introduction of a charset value in convert-source-map@1.4. See thlorenz/convert-source-map@4b8aaf9

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 13, 2017
@hzoo
Copy link
Member

hzoo commented Feb 13, 2017

cc @thlorenz as FYI

@babel-bot
Copy link
Collaborator

Hey @loganfsmyth! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@codecov-io
Copy link

codecov-io commented Feb 13, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@1c1e9c7). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5302   +/-   ##
=========================================
  Coverage          ?   89.44%           
=========================================
  Files             ?      204           
  Lines             ?     9956           
  Branches          ?     2692           
=========================================
  Hits              ?     8905           
  Misses            ?     1051           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c1e9c7...e5377d2. Read the comment docs.

@loganfsmyth
Copy link
Member Author

Definitely on the fence as to whether the constitutes breaking, not because I care that our test broke, but just because we have no way of knowing if downstream systems actually properly parse data URLs.

For now my vote is we fix the tests and hope for the best, and if we get reports of sourcemap issues, we can always pin convert-source-map to the old version.

@loganfsmyth loganfsmyth merged commit e1fee21 into babel:master Feb 13, 2017
@loganfsmyth loganfsmyth deleted the fix-sourcemap-test branch February 13, 2017 22:37
@thlorenz
Copy link

thlorenz commented Feb 14, 2017

The charset value is actually a fix, shouldn't break anything which is why we published as a minor.
I can understand your test failed, but you shouldn't get any sourcemap issues.

If you run into any problems please LMK.
However with the charset provided it will fix problems with Chinese characters in the code for instance.

@Delagen actually provided that change, so he may be able to provide more background.

we can always pin convert-source-map to the old version.

I wouldn't recommend that as then you'll basically avoid the fix.
Instead I'd tell the upstream modules to fix their code to parse things correctly.
FWIW you could pin convert-source-map to this major and publish a new major with the later version in order to avoid any compat complaints.

@loganfsmyth
Copy link
Member Author

Yeah I'm not going to worry about it unless we actually hear of issues. I totally agree with you, and it's always hard to decide what constitutes "breaking" when a bugfix might cause issues for people who weren't parsing things properly to begin with.

danez added a commit that referenced this pull request Feb 14, 2017
* Add new flow preset (#5288)

* Fix PathHoister hoisting JSX member expressions on "this". (#5143)

The PathHoister ignored member references on "this", causing it
to potentially hoist an expression above its function scope.

This patch tells the hoister to watch for "this", and if seen,
mark the nearest non-arrow function scope as the upper limit
for hoistng.

This fixes #4397 and is an alternative to #4787.

* Fix PathHoister hoisting before bindings. (#5153)

Fixes #5149 and enables a few additional safe hoists.

* Fix linting error

* feature: Support pure expressions in transform-react-constant-elements (#4812)

* Fix loose for-of with label (#5298)

* Rewrite Hub as interface #5047 (#5050)

* Rewrite Hub as interface #5047

* Update index.js

* Avoid adding unnecessary closure for block scoping (#5246)

When you write

```
for (const x of l) {
  setTimeout(() => x);
}
```

we need to add a closure because the variable is meant to be block-scoped and recreated each time the block runs. We do this.

However, we also add the closure when no loop is present. This isn't necessary, because if no loop is present then each piece of code runs at most once. I changed the transform to only add a closure if a variable is referenced from within a loop.

* Add greenkeeperio-bot to mention-bot blacklist (#5301) [skip ci]

* Upgrade lerna to current beta. (#5300)

* Revert "Upgrade lerna to current beta." (#5303)

* Add charset so tests work with convert-source-map@>1.4 (#5302)

* Add CHANGELOG for 6.23.0 [skip ci] (#5304)

* Update babel-types README from script.

* v6.23.0

* Revert change that lerna force-committed.

* Revert "Rewrite Hub as interface #5047" (#5306)

* v6.23.1

* Revert lerna again
rwjblue added a commit to rwjblue/broccoli-babel-transpiler that referenced this pull request Mar 10, 2017
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants