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

New: ESLint v4 #241

Merged
merged 1 commit into from
Jun 21, 2017
Merged

New: ESLint v4 #241

merged 1 commit into from
Jun 21, 2017

Conversation

gyandeeps
Copy link
Contributor

@gyandeeps gyandeeps commented Jun 16, 2017

Question:

  • Should we remove ESLint v1?
  • Maybe v2 also?

@RReverser
Copy link
Collaborator

Been thinking about removing ESLint v1/v2 too. I'm currently undertaking a generic cleanup of ASTExplorer, if you want, I can remove them as part of it too.

@fkling
Copy link
Owner

fkling commented Jun 16, 2017

I'd advocate for hiding them from the menu, but not completely removing them. Older shared snippets might rely on them.

I guess the general question is: Do we want to make old snippets continue to work and/or is it worth it?

That's one of the reason why I started working on #197.

@gyandeeps
Copy link
Contributor Author

gyandeeps commented Jun 16, 2017

Sounds good.
I think this PR is ready from my side. regardless of v1/v2 convo this PR can still go on.

@hzoo
Copy link
Collaborator

hzoo commented Jun 16, 2017

if it's not that hard to maintain can just leave it unless someone really wants to remove?

@RReverser
Copy link
Collaborator

RReverser commented Jun 16, 2017

@hzoo Yeah but they contribute to the chunks sizes, and keeping them means periodically ensuring that they don't bloat up chunks with important JS and that they + their deps are loaded only on-demand.

Do we want to make old snippets continue to work and/or is it worth it?

@fkling Should we make this a poll on Twitter or something?

@RReverser
Copy link
Collaborator

Btw, are there any major API incompatibilities for plugins between ESLint v1..v4? Or can we just auto-switch to the new ESLint for old snippets?

@gyandeeps
Copy link
Contributor Author

We dont use plugins in here, are you asking about rules?

@RReverser
Copy link
Collaborator

Yeah, custom rules (aren't they usually shipped as plugins in ESLint?).

@gyandeeps
Copy link
Contributor Author

Right, but here we dont have to worry about that aspect as we directly tell eslint to load a rule by giving it a name and the object.
Outer object structure is same across version (as per my memory) but internal methods provided and their implementations have changed. ie the methods on the arguments provided to each node type listeners in the rule.

@RReverser
Copy link
Collaborator

@gyandeeps I see, so we can't just swap them...

@gyandeeps
Copy link
Contributor Author

I think so... Maybe in some cases the inner workings of the rules might break based on certain other functionality changes..

@fkling
Copy link
Owner

fkling commented Jun 17, 2017

I think it would make sense to only expose the latest version in the frontend but be able to build/serve older versions as needed. The work that I started in #197 would allow us to do that.
I'm planning to continue working on this soon.

In 95c53e5 I added the ability to hide parsers/tools from the menu which is what we can do for the different eslint versions for now.

@fkling
Copy link
Owner

fkling commented Jun 21, 2017

Looking at the changes to the lock file (https://gist.github.com/fkling/26a3ad59b43eab66a2065883bb8bfa83) it seems there are a couple of dependencies added (and therefore probably bundled) that are CLI related and therefore not necessary?

Might be worth updating webpack.config.js somehow to ignore those.

edit: Or I guess they are not bundled because you are only importing a subset of eslint... I will go ahead and merge this. We can always optimize later if necessary.

@fkling fkling merged commit 099d653 into fkling:master Jun 21, 2017
@fkling fkling added the deploy pending Marks issues/PRs that are merged into master but are not deployed to any webserver label Jun 21, 2017
@gyandeeps gyandeeps deleted the eslint-4 branch June 21, 2017 02:16
@fkling fkling added deployed to production Marks issues/PRs that are deployed to https://astexplorer.net and removed deploy pending Marks issues/PRs that are merged into master but are not deployed to any webserver labels Jun 21, 2017
@RReverser
Copy link
Collaborator

RReverser commented Jun 21, 2017

Actually this breaks production build (reminds me the reason I wanted to use it for Travis in #243, cc @josephfrazier):

ERROR in ./~/eslint4/~/eslint/lib/util/source-code.js
Module build failed: SyntaxError: Unexpected token name «i», expected punc «;»
    at JS_Parse_Error.get (eval at <anonymous> (/Users/rreverser/oss/astexplorer/website/node_modules/uglify-js/tools/node.js:1:1), <anonymous>:86:23)
    at new ModuleBuildError (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/lib/ModuleBuildError.js:18:42)
    at runLoaders (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/lib/NormalModule.js:192:19)
    at /Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:364:11
    at /Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:230:18
    at runSyncOrAsync (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:143:3)
    at iterateNormalLoaders (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:229:2)
    at Array.<anonymous> (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:202:4)
    at Storage.finished (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:38:15)
    at /Users/rreverser/oss/astexplorer/website/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:69:9
    at /Users/rreverser/oss/astexplorer/website/node_modules/graceful-fs/graceful-fs.js:78:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:490:3)
 @ ./~/eslint4/index.js 1:87-125
 @ ./src/parsers/js/transformers/eslint4/index.js
 @ ./src/parsers ^\.\/(?!utils)[^\/]+\/(transformers\/([^\/]+)\/)?(codeExample\.txt|[^\/]+?\.js)$
 @ ./src/parsers/index.js
 @ ./src/storage/gist.js
 @ ./src/app.js

ERROR in ./~/eslint4/~/eslint/lib/linter.js
Module build failed: SyntaxError: Unexpected token: operator (>)
    at JS_Parse_Error.get (eval at <anonymous> (/Users/rreverser/oss/astexplorer/website/node_modules/uglify-js/tools/node.js:1:1), <anonymous>:86:23)
    at new ModuleBuildError (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/lib/ModuleBuildError.js:18:42)
    at runLoaders (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/lib/NormalModule.js:192:19)
    at /Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:364:11
    at /Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:230:18
    at runSyncOrAsync (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:143:3)
    at iterateNormalLoaders (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:229:2)
    at Array.<anonymous> (/Users/rreverser/oss/astexplorer/website/node_modules/loader-runner/lib/LoaderRunner.js:202:4)
    at Storage.finished (/Users/rreverser/oss/astexplorer/website/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:38:15)
    at /Users/rreverser/oss/astexplorer/website/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:69:9
    at /Users/rreverser/oss/astexplorer/website/node_modules/graceful-fs/graceful-fs.js:78:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:490:3)
 @ ./~/eslint4/index.js 1:13-41
 @ ./src/parsers/js/transformers/eslint4/index.js
 @ ./src/parsers ^\.\/(?!utils)[^\/]+\/(transformers\/([^\/]+)\/)?(codeExample\.txt|[^\/]+?\.js)$
 @ ./src/parsers/index.js
 @ ./src/storage/gist.js
 @ ./src/app.js
...
error Command failed with exit code 2.

@RReverser
Copy link
Collaborator

RReverser commented Jun 21, 2017

@fkling Hmm, but you pushed the new website - did this error occur during build for you too?

@RReverser
Copy link
Collaborator

Actually, the reason is deeper - eslint4 is left completely untranspiled in the production. Fixing now.

@gyandeeps
Copy link
Contributor Author

sorry about this. I had no idea about all this. I would have done something if I had known about these issues. Thanks @RReverser 💯

@RReverser
Copy link
Collaborator

@gyandeeps No, that's okay, just something that needs to be detected and fixed on our side better.

@josephfrazier
Copy link
Contributor

Actually this breaks production build (reminds me the reason I wanted to use it for Travis in #243, cc @josephfrazier)

Ah, I guess we should switch that over.

RReverser added a commit that referenced this pull request Jun 21, 2017
Fixes a production build error introduced by #241.
@RReverser
Copy link
Collaborator

Fixed in dcdcae5

@RReverser
Copy link
Collaborator

@josephfrazier @fkling I've added two different env to let Travis run NODE_ENV=development and NODE_ENV=production versions in parallel. This should be enough to reproduce these kinds of issues and shouldn't slow down us too much.

81168f73-be72-4ca5-b127-b45c02fb48f9

@fkling
Copy link
Owner

fkling commented Jun 21, 2017

Uh I fixed that locally but I guess I forgot to push my changes. Sorry about that 😞

@josephfrazier
Copy link
Contributor

You'll probably want to use yarn install --production=false in the install script, to ensure that all the build dependencies are still pulled in. See https://yarnpkg.com/en/docs/cli/install#toc-yarn-install-production

@RReverser
Copy link
Collaborator

@josephfrazier Ah yeah makes sense...

@RReverser
Copy link
Collaborator

@josephfrazier
Copy link
Contributor

Sure thing @RReverser, glad to see it's working now (with a README badge, too)!

ionisaligox72 added a commit to ionisaligox72/astexplorer that referenced this pull request Aug 7, 2024
Fixes a production build error introduced by fkling/astexplorer#241.
my2 added a commit to my2/astexplorer that referenced this pull request Aug 13, 2024
Fixes a production build error introduced by fkling/astexplorer#241.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed to production Marks issues/PRs that are deployed to https://astexplorer.net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants