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

fix: monaco-graphql exports, validate on load #3384

Merged
merged 9 commits into from
Aug 12, 2023
Merged

fix: monaco-graphql exports, validate on load #3384

merged 9 commits into from
Aug 12, 2023

Conversation

acao
Copy link
Member

@acao acao commented Aug 2, 2023

  • validates both graphql and json variables on load
  • reduces bundle size further, allowing users to specify which features they want, instead of importing all of them
  • adds a typescript mode to the vite/next.js demos to ensure our implementation doesn't break typescript language mode & worker
  • fix variables json validation by pointing to a meta schema
  • add exports aliases that are backwards compatible, but allow monaco-graphql/initializeMode and monaco-graphql/graphql.worker to be imported directly
  • add a monaco-graphql/lite import for minimal features
  • import the json language mode only if variables json validation is used

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

🦋 Changeset detected

Latest commit: 1c815c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
monaco-graphql Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -19,4 +19,14 @@
import 'monaco-editor/esm/vs/basic-languages/graphql/graphql.contribution.js';
import 'monaco-editor/esm/vs/language/json/monaco.contribution.js';

export * from 'monaco-editor/esm/vs/editor/edcore.main.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

edcore.main.js imports editor.all.js, which includes many features we don't even use yet!

instead, we implement only the minimum features we need for our mode to work, and users can opt in to the rest

@@ -13,7 +13,7 @@ describe('monaco-editor', () => {
// expect(lines[0]).toBe('$ vite build');
// expect(lines[1]).toMatch(' building for production...');
// expect(lines[2]).toBe('transforming...');
expect(lines[3]).toMatch('✓ 970 modules transformed.');
expect(lines[3]).toMatch('✓ 839 modules transformed.');
Copy link
Member Author

Choose a reason for hiding this comment

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

reduces the total modules by replacing edcore.main.js, even though we add the typescript mode to the vite bundle!

@netlify
Copy link

netlify bot commented Aug 2, 2023

Deploy Preview for graphiql-test ready!

Name Link
🔨 Latest commit 4adf2b4
🔍 Latest deploy log https://app.netlify.com/sites/graphiql-test/deploys/64cb844d19e1b90008828fb8
😎 Deploy Preview https://deploy-preview-3384--graphiql-test.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

The latest changes of this PR are available as canary in npm (based on the declared changesets):

@acao
Copy link
Member Author

acao commented Aug 3, 2023

@bboure feel free to give the canary release here a try!

@bboure
Copy link
Contributor

bboure commented Aug 4, 2023

Here are some issues I found:

import { initializeMode } from 'monaco-graphql/full'; does not work with error Module not found: Error: Default condition should be last one

Same error with import { initializeMode } from 'monaco-graphql/esm/lite'; and import { initializeMode } from 'monaco-graphql/esm/initializeMode';

import { initializeMode } from 'monaco-graphql/esm/full'; does not work either. full does not export anything

import { initializeMode } from 'monaco-graphql/lite'; does not work with error Module not found: Error: Package path ./lite is not exported from package

import { initializeMode } from 'monaco-graphql/initializeMode'; does not error at runtime but VScode says Cannot find module 'monaco-graphql/initializeMode' or its corresponding type declarations.

I think all these errors are related to the exports in package.json

With the following config in my project:

import * as monaco from 'monaco-editor'; // force fully featured monaco
import { initializeMode } from 'monaco-graphql/initializeMode';

initializeMode();
new MonacoWebpackPlugin({
      languages: ['json', 'typescript', 'graphql'],
      features: ['!readOnlyMessage'],
      customLanguages: [
        {
          entry: undefined,
          label: 'graphql',
          worker: {
            id: 'graphql',
            entry: require.resolve('monaco-graphql/esm/graphql.worker.js'),
          },
        },
      ],
    }),

Graphql features don't work: autocomplete, validation, formatting...
Other languages work. e.g. typescript

Speaking of typescript, at some point, I also faced that issue that I never had before.

These are just a few issues I met.
tbh, I don't know what is the root cause of all these issues yet. I tried many combinations by upgrading to monaco- editor (from 0.38.0 to 0.40.0 and 0.41.0) and the webpack plugin (from 7.01 to 7.1.0), importing fully -featured monaco vs just the API, etc.

I'll try to do more tests and investigate what causes them later this weekend.

And thanks for the great work!

@acao
Copy link
Member Author

acao commented Aug 4, 2023

there is no /full export if that helps!

strange though, so many of these are working for me with vite and webpack

@acao
Copy link
Member Author

acao commented Aug 4, 2023

@bboure specifying the typescript language to the webpack plugin will break your project, because of a bug with thier webpack plugin. see comments in the webpack config in this PR to learn how to get typescript working. enjoy!

@bboure
Copy link
Contributor

bboure commented Aug 4, 2023

@acao I did see the typescript fix, thanks. and it worked. I was just saying that it used to work for me before.
I could no determine what caused it to not work anymore. It's probably not this package.

Another thing I noticed, but minor. The canary release does not include the updates in graphql-language-service, so I was seeing the schema error too.

FYI, The README shows an example importing full

@acao
Copy link
Member Author

acao commented Aug 4, 2023

@bboure i fixed the readme and added some require exports for cjs, which may be needed if you are using node 16 or so.

scripts/canary-release.js Outdated Show resolved Hide resolved
@acao
Copy link
Member Author

acao commented Aug 9, 2023

@B2o5T this is important as it includes several key bugfixes, and is a useful preceeding step to type: module

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #3384 (1c815c9) into main (2348641) will increase coverage by 0.10%.
Report is 6 commits behind head on main.
The diff coverage is 98.38%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3384      +/-   ##
==========================================
+ Coverage   55.75%   55.85%   +0.10%     
==========================================
  Files         110      110              
  Lines        5243     5256      +13     
  Branches     1426     1432       +6     
==========================================
+ Hits         2923     2936      +13     
  Misses       1897     1897              
  Partials      423      423              
Files Changed Coverage Δ
...nguage-service/src/utils/getVariablesJSONSchema.ts 94.59% <98.38%> (+0.71%) ⬆️

@acao acao merged commit 31ded5e into main Aug 12, 2023
12 checks passed
@acao acao deleted the fix-monaco-exports branch August 12, 2023 08:15
@acao acao mentioned this pull request Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants