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

Enables jsx plugin in case jsx syntax is used in js files #2530

Merged
merged 7 commits into from Feb 8, 2019

Conversation

lustoykov
Copy link
Contributor

@lustoykov lustoykov commented Jan 11, 2019

↪️ Pull Request

Enables usage of JSX syntax in .js files. There are two other alternative solution proposals apart from the current one (see #2519).

1. Always enable JSX.
Opinion: It's a clean solution, especially in comparison to current one, but I think it would add overhead in terms of speed where jsx is not needed.
2. Catch the error when babel tries to parse, enable jsx and re-build.
Opinion: Babel fails with error code 'BABEL_PARSE_ERROR' that is too generic to identify if it's a jsx problem or not. It's a possible alternative, but also feels a bit hacky.

Feedback appreciated, I intend to add tests if we agree on a solution. I'm not sure if this is a Good First Issue since the solutions we've come up with are not ideal.

💻 Examples

import React from "react";
import ReactDOM from "react-dom";

const MyComponent = () => (
	<button>Hello</button>
);

ReactDOM.render(<MyComponent />, document.getElementById("root"));

JSX-syntax works now even in files with extension .js. For instance, index.js.

🚨 Test instructions

Create index.js file with above contents and run parcel index.js

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@mischnic
Copy link
Member

🚨 Test instructions
Create index.js file with above contents and run parcel index.js

More specifically: in an empty folder (no package.json), create a index.js ...

*/
function isUsingJSXinJS(asset) {
// matches import * as React from 'react' and alike
const es6Candidate = /from\s+[`"'](react|preact|nervjs|hyperapp)[`"']/;
Copy link
Member

Choose a reason for hiding this comment

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

Would this make sense to be just one regex (that is defined as a constant at the top similar to how env variable detections and such works)?

That would simplify this code a lot

if (
pragma ||
JSX_EXTENSIONS[path.extname(asset.name)] ||
isUsingJSXinJS(asset)
Copy link
Member

Choose a reason for hiding this comment

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

Should we also use the regex to determine what pragma to use? Presumably this case is to detect if you haven't already installed one of those dependencies yet so would be weird if it always used react.

Copy link
Contributor Author

@lustoykov lustoykov Jan 12, 2019

Choose a reason for hiding this comment

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

This is a valid concern, I was thinking about it as well. Luckily, the pipeline is built in a way that it would, after creating ast in pretransform, re-collect dependencies, install them and re-iterate over that function which results in setting correct pragma.

On second thought, I've decided to implement your suggestion as it should add robustness.

@@ -0,0 +1,3 @@
{
"private": true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying problem this PR is trying to solve is that JSX in JS support fails when no dependencies are specified.

No dependencies are two-folded

  1. When there are no deps in package.json
  2. When there is no package.json

Unfortunately, I couldn't capture the second case with those tests as omitting package.json would modify package.json in other directories (the top-level package.json in integration-tests I think).

However, creation of package.json is tested on many other occasions, which would lead us back to case 1. So case 2. should work by implication (tested locally, is fine).

@@ -0,0 +1,3 @@
const Preact = require('preact');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not using import here for the sake of testing require syntax as well.

DeMoorJasper
DeMoorJasper previously approved these changes Jan 12, 2019
*/
function maybeCreateFallbackPragma(asset) {
for (const dep in JSX_PRAGMA) {
if (asset.contents.match(createJSXRegexFor(dep))) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should create these regexes ahead of time rather than recreating them for every file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 thanks for the hint, will fix tomorrow morning

@@ -24,9 +24,17 @@ function createJSXRegexFor(dependency) {
* package.json is empty or missing yet and therefore pragma cannot
* be determined based on pkg.dependencies / pkg.devDependencies
*/
const cacheJsxRegexFor = {};
Copy link
Contributor Author

@lustoykov lustoykov Jan 17, 2019

Choose a reason for hiding this comment

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

@devongovett, hope that goes in direction of your proposal. If I understood correctly, the intention behind it was to avoid instantiating the same regex multiple times. Regexes are now lazily created in case needed (in other words: in case maybeCreateFallbackPragma gets called) and then cached (similarly to ahead of time creation).

@DeMoorJasper DeMoorJasper merged commit f23c844 into parcel-bundler:master Feb 8, 2019
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